-
Notifications
You must be signed in to change notification settings - Fork 571
feat(asyncio): Add on-demand way to enable AsyncioIntegration #5288
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
| with _installer_lock: | ||
| if identifier in client.integrations: | ||
| logger.debug("Integration already enabled: %s", identifier) | ||
| return None |
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.
I'm explicitly not allowing overwriting the old integration with the new one so that we don't end up double patching the event loop. We could add extra logic to restore the original code and re-patch that, but it seems like that's a lot of extra work for a usecase no one will probably need.
| client = sentry_sdk.get_client() | ||
|
|
||
| with _installer_lock: | ||
| if identifier in client.integrations: |
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.
Race condition allows double-patching of asyncio event loop
Medium Severity
The _enable_integration function checks client.integrations while holding the lock, but the assignment to client.integrations in enable_asyncio_integration happens after the lock is released. If two threads call enable_asyncio_integration concurrently, both can pass the check (since client.integrations is still empty), causing setup_once() and patch_asyncio() to be called twice. This results in double-wrapping the task factory, directly contradicting the stated goal of preventing double patching. The check could use _installed_integrations (which is updated inside the lock), or the assignment to client.integrations could be moved inside the lock.
Additional Locations (1)
| client = sentry_sdk.get_client() | ||
|
|
||
| with _installer_lock: | ||
| if identifier in client.integrations: |
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.
Missing global check allows duplicate setup_once() after re-init
Medium Severity
The _enable_integration function only checks client.integrations (specific to the current client), but doesn't check _installed_integrations (the global set tracking all successfully installed integrations). The original setup_integrations uses _processed_integrations to prevent calling setup_once() multiple times since it's a static method that patches global state. When a user re-initializes Sentry (calling init() again), the new client has an empty integrations dict, so _enable_integration will call setup_once() again even though the event loop was already patched by a previous client, causing double instrumentation.
Initializing the SDK in an async environment can be a challenge, since the requirement to initialize as early as possible might clash with the requirement to initialize when there's an event loop running, if one wants to use the AsyncioIntegration. These sort of scenarios would benefit from a dedicated way to activate the integration after
init()has been called.Supersedes the needlessly more general #5285