-
Notifications
You must be signed in to change notification settings - Fork 562
fix: Replace asyncio.iscoroutinefunction with compatibility shim for Python 3.16 #4915
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
sentry_sdk/_compat.py
Outdated
iscoroutinefunction = asyncio.iscoroutinefunction # type: ignore[assignment] | ||
|
||
def markcoroutinefunction(func): | ||
# type: (_F) -> _F | ||
func._is_coroutine = asyncio.coroutines._is_coroutine # type: ignore | ||
return func |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential bug: The fallback for markcoroutinefunction
uses asyncio.coroutines._is_coroutine
, which was removed in Python 3.11, causing an AttributeError
.
-
Description: The compatibility shim for
markcoroutinefunction
provides a fallback for Python versions older than 3.12. This fallback incorrectly assumesasyncio.coroutines._is_coroutine
exists. However, this attribute was removed in Python 3.11. When the SDK is used with a Django ASGI application on Python 3.11, the fallback logic is triggered, leading to anAttributeError
whenmarkcoroutinefunction
is called insentry_sdk/integrations/django/asgi.py
. -
Suggested fix: The fallback implementation for
markcoroutinefunction
needs to be updated to not useasyncio.coroutines._is_coroutine
. A different implementation that is compatible with Python versions prior to 3.12, including 3.11, should be used.
severity: 0.85, confidence: 0.95
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @Dibbu-cell.
I have requested some initial changes.
I had a look at other libraries and there are differences at how they detect coroutines across different Python versions. I will take a another look at the changes in sentry_sdk
.
We would be in line with asgiref
with your changes: https://github.com/django/asgiref/blob/f587b122af17bdba5749c30b96d2237bc1c2dfdf/asgiref/sync.py#L61-L69
Starlette does a simpler version check.
https://github.com/Kludex/starlette/blob/3912d6313730cc6004dfb4436e37dbc1a81db7c8/starlette/_utils.py#L11-L15
It's worth investigating if there could be any differences with the methods for detecting full inspect and asyncio compatibility for different Python version.
ok, i will do that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for responding to my review @Dibbu-cell.
I've added two more changes we should implement to ensure we don't break anyone. Looks good otherwise!
PY38 = sys.version_info[0] == 3 and sys.version_info[1] >= 8 | ||
PY310 = sys.version_info[0] == 3 and sys.version_info[1] >= 10 | ||
PY311 = sys.version_info[0] == 3 and sys.version_info[1] >= 11 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switching asyncio.iscoroutinefunction
for inspect.iscoroutinefunction
is tricky because of historical incompatibility. Since we support Python versions 3.6 and up we have to care about this unfortunately.
We should use the most conservative cutoff to not break any users. So while CPython deprecated asyncio.iscoroutinefunction
in version 3.12, we should probably mirror Starlette who use 3.13 because of a bug in standard libraries which have not been backported. See Kludex/starlette#2983
Also, since we vendor _is_async_callable
, the change below would keep the vendor up to date. We can therefore be more confident that we won't run into edge cases, since users of starlette
would have run into them already.
PY313 = sys.version_info[0] == 3 and sys.version_info[1] >= 13 | |
iscoroutinefunction = inspect.iscoroutinefunction if PY313 else asyncio.iscoroutinefunction |
sentry_sdk/integrations/asgi.py
Outdated
return asyncio.iscoroutinefunction(app) | ||
return iscoroutinefunction(app) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No action required, just documenting.
This logic is vendored from uvicorn
and they recently replaced asyncio.iscoroutinefunction
with inspect.iscoroutinefunction
. See Kludex/uvicorn#2659
Since we will check for Python 3.13 and up and uvicorn
seems to be happy using inspect
on 3.9 and up, we can rely on uvicorn
having battle testing this change as well.
I have done the appropriate changes. |
It looks like some unrelated changes snuck in with your most recent commit @Dibbu-cell. Can you please revert everything unrelated to changing git checkout master -- sentry_sdk/integrations/django/asgi.py sentry_sdk/client.py tests/tracing/test_allow_n_plus_one.py Also please revert Thanks! |
I have done the changes and sorry for troubling you. |
I am new in open Source . |
check if there is problem let me know. |
Hi @Dibbu-cell, Don't worry, we're open to accept contributions! It looks like you've reverted too many of your changes. I can only see a change on Since the issue you've tackled affects many integrations and you're new to open source I suggest we narrow the scope. Changes to the integrations also have more edge cases to consider so are more complicated than I initially anticipated. Could you only change |
Description
This PR fixes the deprecation warning for asyncio.iscoroutinefunction that will be removed in Python 3.16. The function was deprecated in Python 3.12 in favor of inspect.iscoroutinefunction.
Issues
Reminders
tox -e linters
.feat:
,fix:
,ref:
,meta:
)