-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Fix local variables capturing for out-of-app frames #17545
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
Conversation
...etsentry#12588) This commit addresses an issue where local variables were not being captured for out-of-app frames, even when the `includeOutOfAppFrames` option was enabled. The `localVariablesSyncIntegration` had a race condition where it would process events before the debugger session was fully initialized. This was fixed by awaiting the session creation in `setupOnce`. The tests for this integration were failing because they were not setting up a Sentry client, which is required for the integration to be enabled. This has been corrected by adding a client to the test setup. Additionally, this commit adds tests for the `localVariablesAsyncIntegration` to ensure it correctly handles the `includeOutOfAppFrames` option.
...local variables integration
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
... in local variables integration
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: Changing setupOnce to async violates the integration interface contract. The framework calls it synchronously, reintroducing a race condition where events are silently ignored during initialization.
-
Description: The
Integrationinterface definessetupOnceas a synchronous method, but it was changed to beasync. The integration framework calls this method withoutawait, causing the initialization logic inside to run in the background. While this async operation is pending, ashouldProcessEventflag remainsfalse. Consequently, any events received byprocessEventduring this startup window are silently ignored instead of being processed. This reintroduces the exact race condition the change was intended to fix, causing the integration to silently fail to capture variables for an indeterminate period. -
Suggested fix: Revert
setupOnceto a synchronous function to adhere to the interface contract. If asynchronous initialization is required, consider an alternative approach, such as queuing events within the integration until the async setup is complete, rather than making thesetupOncehook itself asynchronous.
severity: 0.85, confidence: 0.98
Did we get this right? 👍 / 👎 to inform future reviews.
The `LocalVariables` integrations `setupOnce` method was `async`, which violates the `Integration` interface. This caused a race condition where events could be processed before the integration was fully initialized, leading to missed local variables. This commit fixes the race condition by: - Making `setupOnce` synchronous to adhere to the interface contract. - Moving the asynchronous initialization logic to a separate `setup` function. - Making `processEvent` asynchronous and awaiting the result of the `setup` function, ensuring that the integration is fully initialized before processing any events. - Updating the tests to correctly `await` the `processEvent` method.
This commit addresses an issue where local variables were not being captured for out-of-app frames.
The
localVariablesSyncIntegrationhad a race condition where it would process events before the debugger session was fully initialized. This was fixed by awaiting the session creation insetupOnce.Additionally, this PR adds tests for the
localVariablesAsyncIntegrationto ensure it correctly handles theincludeOutOfAppFramesoption.--
Before submitting a pull request, please take a look at our
Contributing guidelines and verify:
yarn lint) & (yarn test): The tests failError: Failed to resolve entry for package "@sentry/browser". The package may have incorrect main/module/exports specified in its package.json.