Fix the attach race in HealthEdtDeliveryTest.aFacadeActionDeliversOnTheEdt - #5516
Conversation
…heEdt The test asserted a real property against an unwinnable race. `Health`'s `openHealthSettings` completes its `EdtResult` before it returns, so by the time the test attached `onResult` the EDT had usually already run the queued delivery. `AsyncResource.ready` runs a listener attached after settlement inline on the attaching thread, so the landing recorded "not the EDT" for a delivery that had in fact happened there. The delivery stack confirms it -- the callback fired from `ready`'s already-done branch, not from the queued runnable. The other six tests dodge this by holding the backend open until the listener is attached; the facade has nothing to hold. Park the EDT instead: a blocking runnable queued ahead of the delivery, released once the listener is on. Also corrects the `assertDeliveredOnEdt` docs. They claim these tests run on the EDT and that `invokeAndBlock` keeps the loop pumping. JUnit runs them on `main`, so `invokeAndBlock` takes its non-EDT branch and runs the operation inline while the real EDT pumps alongside -- which is exactly why the race was wide open. Moves the facade test's javadoc back onto the facade test; it had drifted onto `everyPublicHealthResourceDeliversOnTheEdt`. No product change. `EdtResult` was hopping correctly in every failing run. Before: 8/8 failures running the method alone, ~1/10 running the whole class. After: 0/12 alone, 0/30 for the class. SpotBugs/PMD/Checkstyle/Spotless clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba2206130e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Health.getInstance().openHealthSettings().onResult(landing); | ||
| attached.countDown(); |
There was a problem hiding this comment.
If openHealthSettings() or onResult() throws, attached.countDown() is skipped while the EDT remains blocked in await(). Instead of reporting the original failure, teardown's EDT flush or later UI tests can then hang indefinitely. Release the latch in a finally block around the facade call and listener attachment.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR fixes a deterministic/flake race in HealthEdtDeliveryTest#aFacadeActionDeliversOnTheEdt by ensuring the test attaches its result listener before the EdtResult delivery runnable is allowed to run on the EDT. It also updates nearby test documentation/comments to reflect the actual threading behavior of the JUnit harness used here.
Changes:
- Add an EDT “barrier” (via
CountDownLatch+CN.callSerially) to prevent the facade result delivery from racing ahead of listener attachment. - Correct and expand
assertDeliveredOnEdtdocumentation to match how the tests actually execute (JUnitmainthread + a concurrently running EDT). - Move the facade test JavaDoc back onto the facade test method it describes.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| CN.callSerially(new Runnable() { | ||
| public void run() { | ||
| try { | ||
| attached.await(); | ||
| } catch (InterruptedException ex) { | ||
| Thread.currentThread().interrupt(); | ||
| } | ||
| } | ||
| }); | ||
| Health.getInstance().openHealthSettings().onResult(landing); | ||
| attached.countDown(); |
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Codex and Copilot both caught the same defect in the barrier: if `openHealthSettings` or `onResult` threw, `countDown` was skipped and the EDT stayed parked in an unbounded `await`. The suite would then hang rather than report the throw that caused it -- a worse failure mode than the flake being fixed. Release the latch in a `finally`, and bound the wait at 10s as a backstop for anything the `finally` cannot reach. Verified with a throwaway probe replicating the barrier around a deliberate throw: the `IllegalStateException` surfaces as itself, the blocker is released, and a later test's serial call still runs on a live EDT -- in 0.106s, so the `finally` did the releasing and the timeout was never reached. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Good catch — fixed in 1653359. You're right that the failure mode was worse than the flake being fixed: a skipped Applied both suggestions — CN.callSerially(new Runnable() {
public void run() {
try {
attached.await(10, TimeUnit.SECONDS);
} catch (InterruptedException ex) {
Thread.currentThread().interrupt();
}
}
});
try {
Health.getInstance().openHealthSettings().onResult(landing);
} finally {
attached.countDown();
}Verified with a throwaway probe that replicates the barrier around a deliberate throw: The Re-verified after the change: 0/10 failures running the method alone, 0/25 for the full class, and |
Master had independently fixed several of the same defects, usually better, so those resolutions take master's side outright rather than preserving mine. - javase-cef-ffmpeg-smoke.yml / build-android-app.sh: same two fixes, same reasoning. Master's ffmpeg step also runs `ffmpeg -version` to confirm the install rather than trusting choco's exit code, which mine did not. - MCPLoopbackSocketTransport: master reworked the whole open/close protocol around an `opening` claim taken in the same critical section that checks it, and deliberately supports reopening one transport across stop()/start() by clearing `closed` when the claim is taken. My fix refused to claim while closed, which is directly incompatible with that -- so master's design wins and mine is dropped. Master's test file covers the race I was chasing more thoroughly than my test did (stoppingWhileTheReaderThreadIsStillOpening..., restartingWhileTheOldReaderIsStillOpening...), and its premise -- that a closed transport must refuse to reopen -- is no longer true, so my test goes with it. - EdtResult: master hops the registration onto the EDT and documents why `except` must stay synchronous (HealthFallbackTest.errorOf and BtTestUtil read an error by registering a callback and depend on it). My version wrapped `except` too. Master's is the more careful of the two; git had auto-merged both into a duplicate method. - HealthEdtDeliveryTest: master's #5516 added a test with the same name and intent as mine. - BrowserComponentScreenshotTest: both tightened the blank-frame check; master's thresholds are stricter on both axes (a quarter of the band's pixels rather than bandWidth/20, and r<48 rather than r<80), so master's numbers stand with my note about the Linux stub kept alongside them. Verified on the merged tree: 4754 core tests, SpotBugs clean, 26 port-status gate tests, all four touched workflows parse, both sweep scripts parse, copyright gate over 97 files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The problem
HealthEdtDeliveryTest.aFacadeActionDeliversOnTheEdtfails 8/8 times when run on its own, and flakes roughly 1 in 10 when the whole class runs:pr.ymlrunscore-unittestson every PR, so this was an intermittent red build waiting to happen.Root cause
The product code is correct —
EdtResulthops to the EDT in every failing run. The test loses a race it cannot win.Health.openHealthSettings()completes itsEdtResultbefore it returns:The test then attaches
.onResult(landing)afterwards. By then the EDT has usually already run the queuedDeliver, andAsyncResource.readytakes its already-settled branch (AsyncResource.java:451), invoking the listener inline on the attaching thread. The landing recordsisEdt() == falsefor a delivery that genuinely happened on the EDT.Instrumenting the delivery proves it fires from the attach, not from the queued runnable:
This is the same race the class already documents on
anAggregateDeliversOnTheEdt. The other six tests dodge it by holding the backend (holdRead/holdWrite/holdAggregate/holdDelete) until the listener is attached. The facade settles synchronously, so there is nothing to hold — which is why this one test was left exposed.A second detail widens the window: JUnit runs these on
main, not the EDT, soCN.invokeAndBlocktakes its non-EDT branch (Display.java:1561, a plainr.run()). The real EDT spins idle alongside and grabs the queued delivery almost instantly. That is why isolation fails deterministically while a busier full-class run usually wins.The fix
Park the EDT behind a barrier queued ahead of the delivery, and release it once the listener is attached. Serial calls run FIFO, so the ordering is guaranteed rather than hoped for.
Also in this change:
assertDeliveredOnEdtdocs. They stated these tests run on the EDT and thatinvokeAndBlockkeeps the loop pumping. Neither holds, and the gap between the comment and the behaviour is what made the race invisible. Replaced with what actually happens, plus the attach-before-settle rule every test here depends on.everyPublicHealthResourceDeliversOnTheEdt's own javadoc, documenting a test ~100 lines away.No production code is touched.
Verification
-Dtest=HealthEdtDeliveryTest#aFacadeActionDeliversOnTheEdt-Dtest=HealthEdtDeliveryTest(full class)mvn verifyoncore-unittestsis green: SpotBugsBugInstance size is 0, PMD, Checkstyle and Spotless all clean.🤖 Generated with Claude Code