-
Notifications
You must be signed in to change notification settings - Fork 572
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
| from threading import Lock | ||
| from typing import TYPE_CHECKING | ||
|
|
||
| import sentry_sdk | ||
| from sentry_sdk.utils import logger | ||
|
|
||
| if TYPE_CHECKING: | ||
|
|
@@ -279,6 +280,28 @@ def setup_integrations( | |
| return integrations | ||
|
|
||
|
|
||
| def _enable_integration(integration: "Integration") -> "Optional[Integration]": | ||
| identifier = integration.identifier | ||
| 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 commentThe reason will be displayed to describe this comment to others. Learn more. Missing global check allows duplicate
|
||
| logger.debug("Integration already enabled: %s", identifier) | ||
| return None | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
|
||
| logger.debug("Setting up integration %s", identifier) | ||
| _processed_integrations.add(identifier) | ||
| try: | ||
| type(integration).setup_once() | ||
| integration.setup_once_with_options(client.options) | ||
| except DidNotEnable as e: | ||
| logger.debug("Did not enable integration %s: %s", identifier, e) | ||
| return None | ||
| else: | ||
| _installed_integrations.add(identifier) | ||
| return integration | ||
|
|
||
|
|
||
| def _check_minimum_version( | ||
| integration: "type[Integration]", | ||
| version: "Optional[tuple[int, ...]]", | ||
|
|
||
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_integrationfunction checksclient.integrationswhile holding the lock, but the assignment toclient.integrationsinenable_asyncio_integrationhappens after the lock is released. If two threads callenable_asyncio_integrationconcurrently, both can pass the check (sinceclient.integrationsis still empty), causingsetup_once()andpatch_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 toclient.integrationscould be moved inside the lock.Additional Locations (1)
sentry_sdk/integrations/asyncio.py#L175-L176