fix(dart): Make sentryOnError synchronous in runZonedGuarded#3697
Open
theprantadutta wants to merge 2 commits into
Open
fix(dart): Make sentryOnError synchronous in runZonedGuarded#3697theprantadutta wants to merge 2 commits into
theprantadutta wants to merge 2 commits into
Conversation
The handler passed to `runZonedGuarded` inside `SentryRunZonedGuarded.sentryRunZonedGuarded` was declared `async`, returning a `Future<void>` that the framework silently coerced to `void` and discarded. Two consequences followed: 1. `_captureError` was awaited inside an unawaited future, so the user-supplied `onError` only ran after the awaited microtask resolved. 2. If the user's `onError` rethrew the error (a common pattern to preserve normal crash output), the throw became an uncaught async error of the same zone and recursively re-entered `sentryOnError` until Dart silently dropped it — swallowing the unhandled exception entirely. Make the handler synchronous, fire `_captureError` via `unawaited`, and call the user's `onError` synchronously so a rethrow propagates out through `runZonedGuarded` instead of looping. The internal synchronous prefix of `_captureError` (including the span-status update and the call into `hub.captureEvent`) is unchanged, so the existing tests that observe scope and event state continue to pass. Closes getsentry#3541
buenaflor
reviewed
May 13, 2026
| await span?.finish(); | ||
| }); | ||
|
|
||
| test( |
Contributor
There was a problem hiding this comment.
you can add the reference to the issue regression as a comment instead
Comment on lines
+96
to
+108
| // Before the fix `sentryOnError` was declared `async`, returning a | ||
| // `Future<void>` that `runZonedGuarded` discarded. The user's | ||
| // `onError` only ran after `await _captureError(...)` resolved on | ||
| // a later microtask, which meant a rethrow from `onError` became | ||
| // an uncaught async error of the same zone — recursively | ||
| // re-entering `sentryOnError` until Dart silently dropped it. | ||
| // | ||
| // After the fix `sentryOnError` is synchronous: `_captureError` is | ||
| // fire-and-forget via `unawaited`, and the user's `onError` runs | ||
| // (and may rethrow cleanly) before `sentryRunZonedGuarded` | ||
| // returns. The clearest sync-vs-async discriminator is whether | ||
| // the user's onError has been invoked by the time control returns | ||
| // to the caller. |
Contributor
There was a problem hiding this comment.
don't think all this comment is needed
Comment on lines
+123
to
+137
| // Synchronous snapshot: with the sync handler this must already | ||
| // be `true`; with the broken async handler it would still be | ||
| // `false` because `await _captureError(...)` had not yet | ||
| // resolved. | ||
| userOnErrorCalledSyncFromCaller = userOnErrorCalled; | ||
|
|
||
| expect(userOnErrorCalledSyncFromCaller, isTrue, | ||
| reason: "sentryOnError must invoke the user's onError synchronously, " | ||
| "not after an awaited microtask. Otherwise a rethrow from " | ||
| "onError becomes an unhandled async error of the same zone " | ||
| "and recursively re-enters sentryOnError until Dart drops " | ||
| "the error."); | ||
|
|
||
| // The unawaited `_captureError` still reaches the client; we just | ||
| // observe it after microtasks drain. |
Contributor
There was a problem hiding this comment.
same here, we can remove these comments, they're quite verbose imo
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3697 +/- ##
=======================================
Coverage 86.96% 86.96%
=======================================
Files 335 335
Lines 11976 11976
=======================================
Hits 10415 10415
Misses 1561 1561
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
- Move "regression for getsentry#3541" out of the test name into a leading comment. - Drop the long bug-history paragraph; the issue link covers it. - Drop the "we just observe it after microtasks drain" comment. - Simplify the `reason` text and inline-variable comment.
Author
|
Thanks for the review @buenaflor! Addressed all three notes in
All 5 tests still pass locally. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📜 Description
SentryRunZonedGuarded.sentryRunZonedGuardedwraps the user'sonErrorinsentryOnError, which is passed to Dart'srunZonedGuarded. The wrapper was declaredasync, so it returned aFuture<void>that the framework silently coerced tovoidand discarded.Two consequences:
_captureError(...)was awaited inside an unawaited future, which meant the user-suppliedonErroronly ran after the awaited microtask resolved (rather than synchronously during error dispatch).onErrorrethrew the error — a common pattern to preserve normal crash output — the throw became an uncaught async error in the same zone and recursively re-enteredsentryOnErroruntil Dart silently dropped it. The unhandled exception was swallowed entirely (no stack trace printed, no Sentry event guaranteed to have been sent).This PR makes the handler synchronous, fires
_captureErrorviaunawaited, and calls the user'sonErrorsynchronously so a rethrow propagates out throughrunZonedGuardedinstead of looping back into itself.The synchronous prefix of
_captureError— theoptions.logcall, span-status update viahub.configureScope, and the call intohub.captureEvent— all still run before the firstawait, so the existing tests that observe scope and event state continue to pass.💡 Motivation and Context
Closes #3541
💚 How did you test it?
invokes user onError synchronously and captures the event (regression for #3541)) that observes the user'sonErrorsynchronously from the caller frame. With the brokenasynchandler this snapshot wasfalsebecause the user callback only ran afterawait _captureError(...); with the fix it istrue.Expected: true / Actual: <false>), then passes once restored.calls onError,calls zoneSpecification print,marks transaction as internal error if no status,sets level to error instead of fatal) all continue to pass — the synchronous prefix of_captureErrorstill sets the scope/span state before the existing tests observe it.dart analyze --fatal-warningsonpackages/dart/— clean.hub_test.dartalongside to check for nearby regressions — all 72 tests pass.📝 Checklist
sendDefaultPiiis enabledvoid Function(Object, StackTrace), the async return was being discarded)