Repository navigation
GROOVY-12763, GROOVY-12764: async constructs rethrow checked exceptions as themselves; no self-suppression - #3298
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3298 +/- ##
==================================================
+ Coverage 74.4894% 74.4908% +0.0014%
- Complexity 44906 44913 +7
==================================================
Files 1703 1703
Lines 152180 152212 +32
Branches 29166 29162 -4
==================================================
+ Hits 113358 113384 +26
- Misses 28408 28413 +5
- Partials 10414 10415 +1
🚀 New features to boost your workflow:
|
daniellansun
left a comment
daniellansun
left a comment
There was a problem hiding this comment.
Thanks @paulk-asert for tackling both GROOVY-12763 and GROOVY-12764 so cleanly! Unifying all five async constructs around checked-exception transparency in a single atomic 6.0.1 change—paired with explicit @apiNote guidance for groovy-concurrent-java callers and the COMPATIBILITY.md entry—is the right architectural call. Given how fresh 6.0.0 is and that async/await and groovy.concurrent are marked incubating, landing all five paths together avoids leaving users in a split state where withScope bodies are transparent while child tasks inside the very same scope stay wrapped.
Left inline notes across the diff; summarizing the main points by priority below:
- Pre-merge fix —
DefaultAsyncScope.withScopeTimeoutself-suppression & timeout cause handling (DefaultAsyncScope.java): UnlikeAsyncScope.withScope(which closesscopemanually withif (closeError != bodyError)),withScopeTimeoutstill usestry (DefaultAsyncScope scope = ...)wherejavacemits an unguardedprimary.addSuppressed(closeError)(JDK-8317229). When a timed body and child task fail with (orawait) the same checked exception instance,withScopeTimeoutnow throwsIllegalArgumentException: Self-suppression not permitted. In addition, callingte.initCause(e)beforeunwrapUndeclared(e)leavesUndeclaredThrowableExceptionas the timeout cause in dynamic Groovy,catch (Exception e)misses direct checkedThrowablesubclasses under@CompileStatic, and a late timer interrupt firing just asbody.apply(scope)returns leaves the caller thread's interrupt flag set. AligningwithScopeTimeoutwithAsyncScope.withScope's manual close structure resolves all of these cleanly. - Pre-merge fix — Preserving
getSuppressed()when stripping wrappers (AsyncSupport.java): BothunwrapandunwrapUndeclaredcurrently returnt.getCause()directly, silently dropping any suppressed exceptions attached to the outer wrapper (for instance, subsequentdeferortry-with-resourcescleanup failures attached to anExecutionExceptionorUndeclaredThrowableExceptionprior to unwrapping). Transferringwrapper.getSuppressed()(withsuppressed != unwrappedguarding) preserves cleanup diagnostics with zero allocation on the common path. - Design & edge-case notes (non-blocking / follow-up):
- Dynamic proxy
UndeclaredThrowableExceptioninunwrapUndeclared: In practice,UndeclaredThrowableException(checked)frequently originates from JDKjava.lang.reflect.Proxyframeworks (Spring AOP, MyBatis, RPC clients) rather than manual instantiation; since it is unchecked, neither dynamicConvertedClosurenor@CompileStaticadds a second wrapper, sounwrapUndeclaredstrips the proxy's wrapper. Deferring entry-point signature work from6.0.1makes sense, and an internalThrowingConsumeroverload onAsyncSupport.asyncGeneratorcould remove SAM coercion for generators cleanly later. - Wrapper & fan-out subtleties:
executeDeferScopeitself keepsfirstErroras-is (ExecutionExceptionis stripped inawait asyncbyawait, whereas inasyncgeneratorsGeneratorBridgepreserves it viaunwrapUndeclared), and a small identity-deduplicating helper foraddSuppressedinDefaultAsyncScope.close()andexecuteDeferScopewould avoid duplicate suppressed entries when multiple fan-out tasks/defers rethrow a shared secondaryerrB != firstError. - Cross-thread observability & interrupt state (future consideration): Rethrowing a worker thread's checked
InterruptedExceptiononto an awaiting thread leavesThread.currentThread().isInterrupted() == false, and bare sneaky-rethrow across worker/virtual threads omits caller-side join frames from the stack trace—worth tracking as a separate follow-up alongside Groovy 7awaitwrapper narrowing.
- Dynamic proxy
- Tests &
6.0.1backport: Expanding@CompileStaticcoverage inAsyncAwaitTestto includefor await(Flow.Publisher) and timedwithScope, plus regression cases for timed self-suppression and wrapper-suppressed transfer, would lock down all paths. Since reversingtestScopeCloseWithCheckedExceptionWrappedalters tested6.0.0behavior in a patch release, a brief heads-up ondev@prior to backporting toGROOVY_6_0_Xmay also be worthwhile if not already sent.
…not permitted" when one exception instance is reported twice Record a later failure as suppressed only when it is not the first failure itself and not already recorded, so a shared instance is neither suppressed into itself nor listed twice. withScope with a timeout now closes its scope by hand, as withScope does, because try-with-resources suppressed a failure shared by the body and a child into itself. The timer hand-off is also tightened so a timer firing as the body finishes cannot leave the caller interrupted.
…ons instead of rethrowing them as await does Scope close, withScope, defer, generators and Flow publishers now rethrow a checked exception as itself, as await does. The UndeclaredThrowable layer that closure coercion adds around a checked exception is removed; other wrappers are kept. A timed withScope uses the unwrapped exception as the cause of its TimeoutException. Exceptions suppressed by a wrapper that is removed move to its cause, since without the old RuntimeException layer they would otherwise be lost.
d6edcaf to
62b1bd6
Compare
daniellansun
left a comment
daniellansun
left a comment
There was a problem hiding this comment.
Thanks @paulk-asert — the updates in d667d75 and 62b1bd6 address every point from the initial review thoroughly and cleanly!
In particular, the 4-state TIMER_PENDING / TIMER_STOPPED / TIMER_FIRING / TIMER_FIRED handshake in DefaultAsyncScope.stopTimer nicely closes even the narrow scheduler window where scope.cancelAll() wakes the body thread just before bodyThread.interrupt() runs, and addSuppressedOnce + transferSuppressed (synchronizing on primary to match Throwable's own monitor) handles concurrent await unwrapping cleanly without lock-order hazards. The expanded @apiNote / COMPATIBILITY.md notes and the new @CompileStatic, shared-suppression, and timer tests in AsyncAwaitTest look great as well.
Only two tiny, strictly non-blocking observations (no need to hold up merging):
- In
AsyncSupport.addSuppressedOnce, checkingif (secondary == primary || secondary == primary.getCause()) return;could avoid attachingcloseErrorintoTimeoutException.getSuppressed()in the rare race where a timed scope expires right as both body and child fail with the same instanceerr(te.initCause(err)+te.addSuppressed(err)causesThrowable.printStackTrace()'sdejaVuset to printCaused by: [CIRCULAR REFERENCE: ...]). - In
testTimedScopeDoesNotLeaveCallerInterrupted, spinningwhile (!Thread.currentThread().isInterrupted() && System.nanoTime() < end) { Thread.onSpinWait() }(with e.g. a 5s ceiling instead of a fixed 300ms busy loop) would finish in ~20ms as soon as the timer interruptsbodyThreadwhile guaranteeing theTIMER_FIREDbranch is exercised even on a heavily stalled CI runner.
+1 LGTM — ready for master and GROOVY_6_0_X!
…sed exceptions are combined addSuppressedOnce also skips an exception that, without its JDK wrapper layers, is the primary itself or the primary's cause. A timed scope that times out as its body and a child fail with one instance no longer lists the TimeoutException's cause among its suppressed exceptions, and moving suppressed exceptions no longer attaches a wrapper to the failure it wraps. Awaitable.first records each failure unwrapped and once. A pending GroovyPromise reads a failure through join(), as a completed one does, because CompletableFuture.get() drops a CompletionException's suppressed exceptions. The timed-scope interrupt test now waits for the interrupt rather than for a fixed time, so it always exercises the fired timer.
|
Thanks Daniel! Both nits are in 3068e71, plus one more from the same family: |
✅ All tests passed ✅🏷️ Commit: 3068e71 Learn more about TestLens at testlens.app/docs. |
Two async fixes for 6.0.1, one commit each.
GROOVY-12763: async constructs rethrow a checked exception as itself
awaitrethrew a checked exception unchanged, but five other paths wrapped it:DefaultAsyncScope.close()) and adeferaction (AsyncSupport.executeDeferScope) inRuntimeExceptionwithScopebody (including the timeout variant) and a generator body inUndeclaredThrowableException, added when the closure is converted toFunction/ConsumerFlow.Publishererror consumed withfor await(FlowPublisherAdapter) inRuntimeExceptionAll five now rethrow the original exception, as
awaitdoes (GROOVY-9381's exception-transparency principle; GROOVY-12459 forfor await). Only theUndeclaredThrowableExceptionlayer that closure conversion adds around a checked exception is removed; other wrappers the code throws are kept. Suppressed exceptions from scope close are kept.AsyncSupport.rethrowis now public and returnsRuntimeExceptionso call sites can writethrow rethrow(t); newAsyncSupport.unwrapUndeclared@apiNoteonAsyncScopeandclose()for Java callers, which can now receive a checked exception these methods don't declaretestScopeCloseWithCheckedExceptionWrapped, which asserted the old wrapping, becomestestScopeCloseRethrowsCheckedExceptionUnwrappedKnown limitation: a checked exception the code itself wrapped in
UndeclaredThrowableExceptionand threw from awithScopeor generator body is also unwrapped. Removing that needs entry points that avoid closure conversion (aClosureoverload ofwithScope, a generator body type declaringthrows Throwable), left for a later release.GROOVY-12764: no "Self-suppression not permitted" when one exception instance is reported twice
Two children (for example, two that await the same failed task) or two
deferactions failing with the same instance madeclose()/executeDeferScopethrowIllegalArgumentExceptionand lose the real failure. They now skipaddSuppressedfor the same instance, aswithScopealready does.Tests
New tests in
AsyncAwaitTestfail before the fix and pass after: each of the five paths, the two self-suppression cases, the paths under@CompileStatic, and wrapper preservation (a wrapper thrown by a child task ordeferaction is removed exactly asawaitremoves it; one from aFlow.Publisheris kept). Locally: full./gradlew test(the one failure was a race in a new Flow test, since fixed), thenAsyncAwaitTest,groovy.concurrent.*andorg.apache.groovy.runtime.async.*green.Both commits are intended for backporting to
GROOVY_6_0_X.