Repository navigation
fix(vertx): every close entry point releases the Vert.x engine - #453
Conversation
`VertxParallelEoSStreamProcessor` only overrode `close(Duration, DrainingMode)`
- the one place `webClient.close()` and `vertx.close()` ran. The no-arg
`close()`, `closeDrainFirst()` and `closeDontDrainFirst()` default methods on
`DrainingCloseable` call `close(DrainingMode)` directly, never the
`Duration`-taking overload, so every one of those entry points bypassed the
Vert.x teardown entirely and stranded the WebClient and the event-loop group.
The teardown also ran after `super.close(...)` with no `finally`, so a
throwing shutdown skipped it regardless of which entry point was used.
Confirmed red first: `VertxCloseReleasesEngineTest` against unfixed master
failed all three cases with "Wanted but not invoked: webClientBase.close() -
Actually, there were zero interactions with this mock" for
`noArgCloseReleasesTheVertxEngine`, `closeDontDrainFirstReleasesTheVertxEngine`
and `aThrowingCloseStillReleasesTheVertxEngine` (verified by stashing just the
main-code fix and re-running the test module).
Fix: override `close(DrainingMode)` instead of `close(Duration, DrainingMode)`.
`close(DrainingMode)` is the single method every entry point funnels through -
including the `Duration`-taking one, whose base-class implementation just
records the timeout in a private field and then calls `close(drainMode)`
virtually. Overriding both would double-run the teardown, because that
base-class call dispatches back to whichever override is more derived. The
Vert.x-side wait is now bounded by `options.getShutdownTimeout()` rather than
a per-call override, since the base class's recorded timeout is a private
field this class has no access to; the trade-off is documented on
`closeVertxEngine()`. The teardown itself now runs in a `finally`, so a
throwing `super.close(...)` still releases the engine.
`VertxBaseUnitTest`'s `@AfterEach closeVertxResources()` workaround - which
called the `Duration`-taking form directly because nothing else reached the
Vert.x teardown - is now redundant and removed: the shared base test class's
own `@AfterEach` already closes the processor via the no-arg `close()`, which
now reaches the same teardown.
Other instances of the same defect class (a subclass overriding only
`close(Duration, DrainingMode)` for its own resource cleanup): checked
`parallel-consumer-reactor`, `parallel-consumer-mutiny` and
`parallel-consumer-core` for a sibling engine that overrides that overload to
release something engine-specific. None found - see the PR body for the
per-module verdicts.
Deletes docs/inflight/bug-vertx-close-skips-engine-cleanup.md, which tracked
exactly this; both of its open questions ("where does the cleanup belong",
"it isn't exception-safe either") are answered by this commit.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
✅ Duplicate Code ReportTwo engines run in parallel for cross-validation. Each has its own thresholds tuned to its baseline - the real safety net is the per-engine "max increase vs base" check. ✅ PMD CPD
No new clones introduced by this PR. ✅ jscpd (language-agnostic)
No new clones introduced by this PR. Powered by astubbs/duplicate-code-cross-check |
|
[superseded - a quarantined test changed outcome] 🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its No quarantined test changed outcome since the previous push. Updated for Superseded by a newer quarantine lane report. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #453 +/- ##
============================================
+ Coverage 81.70% 82.65% +0.94%
- Complexity 1324 1492 +168
============================================
Files 84 95 +11
Lines 4597 5183 +586
Branches 492 510 +18
============================================
+ Hits 3756 4284 +528
- Misses 648 708 +60
+ Partials 193 191 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🟢 Throughput — OKThis branch measured about 13% faster than master, on the one test this measures. That is INSIDE this test's own run-to-run spread of about 17%, so read it as a reading and not as a result - re-running the same commit moves it by about as much.
Allowable range 🟢 ≥ 0.70 · 🟡 0.50–0.70 (about a 30% loss) · 🔴 < 0.50 (about a 50% loss) What the numbers mean, and what they cannot tell youThe one that gets misread. Why a shape and not a rate. A rate depends on which runner you drew. A shape does not: every test here processes a fixed number of records, so a runner twice as slow doubles the subject and the controls together and leaves their ratio alone. That is the whole trick, and it is why the reported rate is shown last and labelled as this machine only. Reading the comparison. By conservation, not by correction. Every test in this lane processes a fixed number of records, so within one run the ratio of one test's time to another's is invariant under machine speed — a runner twice as slow doubles both terms and leaves the ratio alone. There is no machine-index correction to be wrong, because nothing needed correcting. Per-method times, not class times. A class time is Reference is the median of 8 recent What this still cannot do. It removes machine-to-machine variance. It does not remove this test's own run-to-run variance, measured at about 30% on a single unchanged commit while its controls stayed within 5%. That is a property of the test, not of the comparison, and no arithmetic here can touch it — which is why the reference is a median and the bounds are deliberately coarse. 🟡 means look at this; only 🔴 is outside the measured spread. Runs used: ce6f39a, 70a88bf, 27211b9, c5dde06, 92364a1, 867c407, 7a8dd92, df3bd71 Since the previous push: ratio 1.069 -> 1.129, share 1.608 -> 1.539, rate 65472 -> 73778 (+12.7%). One push of difference sits inside this test's measured spread - read it as movement, not as a result. Updated for |
…setup `VertxCloseReleasesEngineTest` overrode `initAsyncConsumer` wholesale to get a Mockito spy in front of `Vertx` and `WebClient`, which meant restating the `VertxOptions`, the `maxConcurrency(10)` options-builder step and the `JStreamVertxParallelEoSStreamProcessor` construction verbatim from `VertxBaseUnitTest`. Only the two instances actually differed. The PR's own file-similarity report flagged the pair as a new 33.1% similarity. `VertxBaseUnitTest` now creates those two instances through `createVertx` and `createWebClient`, and `initAsyncConsumer` calls them. The test overrides only those two hooks - the options and the construction stay in one place, so the next change to either cannot land in one copy and miss the other. Behaviour is unchanged: the hooks return exactly what the inlined calls returned, the web client is still built on whatever `createVertx` produced (the spy, in the test's case), and `initAsyncConsumer` is invoked from `setupParallelConsumerInstance`, not a constructor, so the overridable calls run on a fully constructed subclass. Found by the ce-simplify-code pass over this branch. Its other two reviewers (code reuse, efficiency) returned no findings. Nothing was applied that touches the fix itself: the single-overload `close(DrainingMode)` override, the `finally` around the Vert.x teardown, and the `options.getShutdownTimeout()` bound are all as the previous commit left them. Verified with the vertx module suite before and after - 25 tests, 0 failures both times, including `VertxCloseReleasesEngineTest`'s three cases and the three sibling classes that inherit the changed base. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
`AbstractParallelEoSStreamProcessor.shutdownTimeout` starts as the configured `ParallelConsumerOptions#getShutdownTimeout()` and is replaced by the caller's `Duration` in `close(Duration, DrainingMode)`, which then delegates to `close(DrainingMode)`. It was private, so a subclass engine overriding `close(DrainingMode)` to tear down resources of its own could not see the caller's per-close timeout and had to fall back to the configured default - which is why the Vert.x module's `close(Duration, mode)` stopped honouring its `Duration` for the Vert.x-side wait (#453). The class already exposes seven fields to subclasses through `@Getter(PROTECTED)`; this adds the eighth, plus javadoc saying what the value means at each point in a close and why an engine needs it. Nothing else in the processor hierarchy defines a `getShutdownTimeout()` - the only other one in the tree is on `ParallelConsumerOptions`, a different class - so the generated getter collides with nothing (grepped `getShutdownTimeout` across every module's main and test sources). The first consumer is the Vert.x fix that follows this commit. On the decomposition this file is tracked for (docs/refactoring.md): not taken here - this is a one-line annotation on an existing field, not one of the pieces (control loop, state machine, commit orchestration, threading, rebalance listener) that the split is about. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
…d honours the caller's Duration
Three review findings on the `close(DrainingMode)` override this branch
introduced, each pinned red-first. Red proofs below were produced by reverting
only that item's main-code change against the final tests, running
`./mvnw -pl parallel-consumer-vertx -am test -Dtest='VertxClose*Test'`, then
restoring the change; the same run is green with all three in place.
1. Exception safety. The Vert.x teardown ran in an unguarded `finally`, so a
`TimeoutException`, `InterruptedException` or a failing `webClient.close()`
or `vertx.close()` REPLACED whatever `super.close(...)` was already throwing -
the same replacing shape `AbstractParallelEoSStreamProcessor.doClose` guards
each step of its own `finally` against. `close(DrainingMode)` now captures
the close failure, always runs the teardown, and if the teardown throws too,
attaches it to the close failure as suppressed; the teardown's own exception
is thrown only when there is no close failure to outrank it. Written as a
sequence rather than a guarded `finally`, because a `throw` inside a
`finally` is exactly the shape the fix exists to prevent (and Error Prone's
`Finally` check said so on the first draft).
Red: `VertxCloseReleasesEngineTest.aFailingTeardownIsSuppressedOnTheCloseFailureRatherThanReplacingIt`
against the unguarded finally -
Unexpected exception type thrown, expected: <java.util.NoSuchElementException>
but was: <java.util.concurrent.TimeoutException>
Green: the `NoSuchElementException` propagates with the `TimeoutException`
as its single suppressed exception, and the held-open probe verticle's stop
was requested, proving the teardown reached `vertx.close()`.
2. Engine ownership. The three-arg constructor's javadoc says to use it to
share a Vertx runtime with other systems, but the teardown closed the
`Vertx` and `WebClient` unconditionally - so closing PC closed the caller's
shared runtime. Ownership now follows construction: two final flags,
`ownsVertx` and `ownsWebClient`, decided in the one constructor that builds
(`null` in means PC builds and owns that one; an instance in stays the
caller's), and `releaseOwnedVertxEngine` closes only what the flags say PC
built. Two flags rather than one because the two are supplied independently
and the mixed case is real: `JStreamVertxParallelEoSStreamProcessor`'s
one-arg constructor passed `(null, null)`, while this class's one-arg
constructor passed `(Vertx.vertx(), null)` - a caller's engine with a
PC-built client. That one-arg constructor now passes `(null, null)` too, so
its engine is built by the same path, is owned, and gets the
worker-pool-sized `VertxOptions` the three-arg constructor computed but,
for that path, never applied. Every constructor and the two factory
interfaces (`VertxParallelStreamProcessor.createEosStreamProcessor`,
`JStreamVertxParallelStreamProcessor.createEosStreamProcessor`) route to
one of these, so the flags are right for each. Constructor javadoc states
the contract one sentence each way, on both classes.
Red: `VertxCloseLeavesACallerSuppliedEngineRunningTest` against an
unconditional teardown - all 7 cases (every `DrainingCloseable` entry point
plus the mixed case) failed with
org.mockito.exceptions.verification.NeverWantedButInvoked:
webClientBase.close(); / vertx.close();
Green: none of the six entry points touches a caller-supplied engine or
client; a client PC built on the caller's engine is closed (the next
request is refused with `IllegalStateException: Client is closed`) while
the engine underneath stays running.
3. Timeout. `close(Duration, DrainingMode)` no longer honoured its `Duration`
for the Vert.x wait, which was bounded by `options.getShutdownTimeout()`
because the base class's recorded timeout was private - defended in a
javadoc as a deliberate trade-off. The preceding commit exposes it through
`@Getter(PROTECTED)`, so the wait is now bounded by `getShutdownTimeout()`:
the caller's `Duration` when they gave one, the configured default
otherwise. The trade-off javadoc is gone with the trade-off.
Red: `VertxCloseReleasesEngineTest.closeWithADurationBoundsTheWaitForTheEngineByThatDuration`
against the `options.getShutdownTimeout()` bound, with `close(300ms,
DONT_DRAIN)` on an engine held open by a verticle that never completes its
stop -
expected to be less than: PT5S
but was : PT10.078311S
Green: elapsed is a few hundred milliseconds.
Test shape. The engine is now observed through public API rather than a spy,
because Mockito cannot get in front of an instance the processor builds for
itself: an `EngineProbeVerticle` deployed on the engine under test has its
`stop` run when - and only when - the engine begins closing, since Vert.x
undeploys every verticle before shutting down. The same verticle, told to hold
its stop promise, makes `vertx.close()` hang on real API, which is the
sleep-free timeout seam items 1 and 3 needed. `Vertx#nettyEventLoopGroup()`
would have served but is deprecated in 4.5.31. The processor gains
package-private getters for `vertx` and `webClient` for these same-package
tests (the base class's `@Setter(PACKAGE)` precedent, and its reasoning: not
PROTECTED, so no test-only seam on a public class's extension surface).
`VertxCloseTestBase` holds the shared `CloseEntryPoint` enum, the probe, and
the never-started-drain tolerance, so the two ownership halves do not restate
each other.
Also on review:
- `VertxBaseUnitTest` closes the engine and client it builds for the sibling
tests, after the processor closes - whoever builds, closes, and the
processor now correctly leaves a supplied engine alone. The engine goes
second so nothing in flight is cut off under a still-closing processor. Its
`initAsyncConsumer` now carries the typed signature the abstract base
declares, which removes the unchecked-conversion warnings the raw one
produced in every subclass. `VertxBatchTest` runs 49-56s both before and
after this change (measured at the branch tip with the change stashed:
51.4s), so its length is pre-existing.
- The `@AfterEach` in the close tests catches `NoSuchElementException` only -
the exception the never-started-drain defect actually throws - instead of
`Exception`, so any other teardown failure fails the test again.
- That defect's diagnosis is no longer restated in the test javadoc: one
sentence and a pointer to
docs/inflight/bug-close-drain-first-on-a-never-started-instance.md, which
arrives with branch fix/close-shuts-down-worker-pool (commit de616c4 holds
it until then; the javadoc carries the `git show` form so
`bin/check-file-refs.sh` accepts the path).
- The rationale for the `createVertx`/`createWebClient` seams lives once, on
`VertxBaseUnitTest`; the overrides in the close tests say only what they
substitute and why.
Warnings on lines this commit wrote, for `bin/check-pr-analysis-surfaces.sh`:
NullAway reports the `null` arguments in the one-arg constructor's
`this(null, null, options)`. That is the shape `JStreamVertxParallelEoSStreamProcessor`'s
one-arg constructor already had (same warning, pre-existing), and the
repository has no nullability annotation library on its classpath to declare
the parameters `@Nullable` with, so the contract is stated in the constructor
javadoc instead. Left deliberately rather than papered over.
Verified: `bin/ci-unit-test.sh` green across the reactor (core 691 run, 8
skipped - the pre-existing quarantine/disabled set; vertx 36; reactor 23;
mutiny 18; examples all green); `bin/check-all.sh` green except
`check-issue-refs.sh`, which fails on a bare `#116` in the current PR body and
is fixed with the body, not in this tree.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
#453's own CI This is a sighting record, not a diagnosis. #453's `Chaos Pain Suite 4/4` job killed ChaosChurnStormIT.churnStormMeetsSlosAndBalancesLedger with the gating probe INSTANCE_STALL/NO_WORK_COMPLETED (run 33954010366, job 101273950868, head 0afe4c4). The PR's only change under parallel-consumer-core is a @Getter(AccessLevel.PROTECTED) annotation plus javadoc on AbstractParallelEoSStreamProcessor.shutdownTimeout - no behaviour change, and nothing on the poll, commit, rebalance or shutdown path. Every other touched file is in parallel-consumer-vertx (module and tests) or is one docs/inflight/ note; the chaos suite runs core-module tests only, so this branch's diff cannot have caused the firing. Recorded in docs/inflight/bug-857-family.md, which already carries the same signature's dated sighting entries in this shape (seed, replay command, run/job links, control-arm reasoning). docs/inflight/test-857-churn-storm-async-stalls.md was also read as a candidate owner but is an evolving investigation narrative rather than a sighting ledger, and carries no sighting index to cross-link. `bin/inflight.mjs codecov test churnStormMeetsSlosAndBalancesLedger` returned 12 recent runs, all pass (page-bound) - this run's fail-fast probe kill does not appear among them. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its No quarantined test changed outcome since the previous push. Updated for |
|
@claude review this |
|
Claude finished @astubbs's task in 5m 11s —— View job Code Review
SummaryThis is a well-diagnosed, well-tested fix. The core insight is correct: Verified in the code:
One thing worth confirming (not blocking)Before this fix, Minor
Nothing here blocks merging — the fix is correct, the defect class search (Reactor/Mutiny/core) is thorough and its "clean" verdicts check out from reading those classes' |
The Claude review on this PR (#453) flagged, as a non-blocking "worth confirming" note, that close(DrainingMode)'s teardown re-runs webClient.close()/vertx.close() on every entry point with no guard against a repeat call - the base class's own state == CLOSED short-circuit only skips its own drain/wait bookkeeping on a second close, not this override's teardown. The reviewer suspected Vert.x's close is idempotent but noted nothing in the suite verified it. Added closingASecondTimeThroughAnotherEntryPointDoesNotThrow to VertxCloseReleasesEngineTest, closing once via close() then again via closeDontDrainFirst() on the same never-started instance. Verified by manual mutation: temporarily made the second close() throw unconditionally (an instance boolean flag flipped on first entry) - the new test went red, all other tests in the class stayed green - then reverted, confirming the assertion is load-bearing rather than vacuous. Green: Vert.x's close on an already-closed engine/client returns an already-completed future rather than throwing, so no production change was needed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK
1 participant
Description
VertxParallelEoSStreamProcessoronly overrodeclose(Duration, DrainingMode)- the one placewebClient.close()andvertx.close()ran.DrainingCloseable's no-argclose(),closeDrainFirst()and
closeDontDrainFirst()default methods callclose(DrainingMode)directly, never theDuration-taking overload, so every one of those entry points bypassed the Vert.x teardown entirelyand stranded the WebClient and the event-loop group. The teardown also ran after
super.close(...)with no
finally, so a throwing shutdown skipped it regardless of which entry point was used.Fix: override
close(DrainingMode)instead - the single method every entry point funnels through,including the
Duration-taking one (whose base-class implementation just records the timeout in afield, then calls
close(drainMode)virtually, which is exactly why overriding both overloadswould double-run the teardown). That recorded timeout is now exposed to subclasses through a
@Getter(PROTECTED)on the base class, so the Vert.x-side wait is bounded by the caller's ownDurationwhen they closed throughclose(Duration, DrainingMode)and by the configuredshutdownTimeoutotherwise - the per-call override is honoured again. The teardown runs whether ornot
super.close(...)threw, and cannot replace its failure: a teardown that fails while the close isalready failing is attached to that failure as suppressed, and thrown on its own only when there is
no close failure to outrank it. And it closes only what PC built: ownership is decided in the
constructor (
nullin means PC builds and owns that instance; aVertxorWebClientthe callersupplied stays the caller's to close, as the constructor javadoc now states one sentence each way),
so closing PC no longer closes a Vert.x runtime shared with other systems.
VertxBaseUnitTest's@AfterEach closeVertxResources()workaround - which called theDuration-taking form directly because nothing else reached the Vert.x teardown - is gone. In itsplace the test base releases the engine and client it builds itself, after the processor closes:
whoever builds, closes, and the processor now correctly leaves a supplied engine alone.
Prior art
node bin/inflight.mjs prior-art VertxParallelEoSStreamProcessor closeDrainFirst webClient.close vertx.close VertxBaseUnitTest- the only merged PR touchingVertxParallelEoSStreamProcessorat all is fix(jstream)! astubbs#122: the result stream waits for results instead of ending on an empty queue #116 (astubbs#122), which is the PR this bug's own inflight note says found-and-deliberately-deferred this exact issue. No other merged PR, plan or solution write-up covers it.node bin/inflight.mjs note drift docs/inflight/bug-vertx-close-skips-engine-cleanup.md- nothing divergent; the note's only copy is onorigin/master, so no branch already carries a fix.VertxParallelEoSStreamProcessor- same result as above, only fix(jstream)! astubbs#122: the result stream waits for results instead of ending on an empty queue #116.docs/inflight/branch-912-vertx-leak.md(adjacent vertx work,bugs/912-vertx-stream-memory-leak) - no overlap. That branch clears the JStream deque on close (a memory-leak fix with its own guard test,JStreamMemoryLeakTest912); it does not touchclose(DrainingMode)/close(Duration, DrainingMode)routing.VertxParallelEoSStreamProcessor, but only to widensetupWorkerPool's return type fromThreadPoolExecutortoExecutorService(an unrelated virtual-threads/pom change). No collision with the close-routing changes here.Red-test evidence
VertxCloseReleasesEngineTestwas run against unfixed master first (main-code fix stashed, test fileand the
VertxBaseUnitTestcleanup already in place):./mvnw -pl parallel-consumer-vertx -am test -Dtest=VertxCloseReleasesEngineTest -Dsurefire.failIfNoSpecifiedTests=false.All three cases failed identically:
for
noArgCloseReleasesTheVertxEngine,closeDontDrainFirstReleasesTheVertxEngineandaThrowingCloseStillReleasesTheVertxEngine. After restoring the fix, the same three tests pass.Other instances of the same defect
Defect class: a subclass overriding only
close(Duration, DrainingMode)to run its ownresource-specific cleanup, which the no-arg
close()/closeDrainFirst()/closeDontDrainFirst()entrypoints never reach, because those call
close(DrainingMode)directly.parallel-consumer-reactor'sReactorProcessor- overrides onlyclose(Duration, DrainingMode),but the body is a pure
super.close(timeout, drainMode);pass-through with no cleanup logic of itsown. Its scheduler is
Schedulers.boundedElastic()(Reactor's shared global pool) or a caller-suppliedone - never an instance this class owns and must dispose. Clean - there is no cleanup to skip.
parallel-consumer-mutiny'sMutinyProcessor- same shape: an emptyclose(Duration, DrainingMode)pass-through, and its executor is Mutiny's shared
Infrastructure.getDefaultWorkerPool()or acaller-supplied one. Clean, same reasoning.
parallel-consumer-core'sExternalEngineandParallelEoSStreamProcessor- neither overrides anyclose overload at all. N/A.
Vert.x is the only engine that owns an exclusive resource (the
Vertx/WebClientpair) needingsubclass-specific teardown, so it was also the only place this defect could manifest.
(The empty
close(Duration, DrainingMode)overrides inReactorProcessorandMutinyProcessoraredead-code no-ops that could be deleted for clarity - a simplification opportunity, not a bug, and out
of scope here.)
Also in this PR
Deletes
docs/inflight/bug-vertx-close-skips-engine-cleanup.md, which tracked exactly this defect;both of its open questions ("where the cleanup belongs", "it isn't exception-safe either") are
answered by this fix.
Checklist
docs/features/- this is a bugfix to existing behaviour, not a new featuredocs/inflight/working note - the fix and its evidence are fully captured in this PR body and the commit; no cross-branch state needs tracking beyond itce-simplifyandce-code-reviewlocally - both ran; the simplify pass landed as the test-substitution commit, and the review's findings (exception safety, engine ownership, honouring the caller'sDuration, the test teardown and javadoc trims) landed in the two commits that follow it🤖 Generated with Claude Code
https://claude.ai/code/session_019SVm2cT6ZgUyPMim9CukYK