From ba2206130e682605626eafbbb7a7b49da6204adc Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Tue, 4 Aug 2026 10:18:40 +0700 Subject: [PATCH 1/2] Fix the attach race in HealthEdtDeliveryTest.aFacadeActionDeliversOnTheEdt 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) --- .../health/HealthEdtDeliveryTest.java | 66 ++++++++++++------- 1 file changed, 44 insertions(+), 22 deletions(-) diff --git a/maven/core-unittests/src/test/java/com/codename1/health/HealthEdtDeliveryTest.java b/maven/core-unittests/src/test/java/com/codename1/health/HealthEdtDeliveryTest.java index c51fc2e35d6..b83bdbda835 100644 --- a/maven/core-unittests/src/test/java/com/codename1/health/HealthEdtDeliveryTest.java +++ b/maven/core-unittests/src/test/java/com/codename1/health/HealthEdtDeliveryTest.java @@ -34,6 +34,7 @@ import java.util.Map; import java.util.ArrayList; import java.util.List; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.atomic.AtomicBoolean; import static org.junit.jupiter.api.Assertions.*; @@ -72,14 +73,20 @@ public void onReady(T value, Throwable err) { * Runs {@code op} off the EDT and waits for its delivery without blocking * the EDT. * - *

Worth spelling out, because the obvious harness fails in a way that - * looks like a product bug. These tests run on the EDT; the delivery being - * asserted is a {@code callSerially}; so waiting on a latch from the test - * thread stops the event loop and the runnable carrying the result can - * never run. {@code invokeAndBlock} is the CN1 answer -- it moves the - * waiting off the EDT and keeps the loop pumping. My first version of this - * file used a plain latch and reported the aggregate path as broken when - * it was not.

+ *

Worth spelling out, because the harness reads as more than it is. + * JUnit runs these on {@code main}, not on the EDT, so + * {@code invokeAndBlock} takes its non-EDT branch and simply runs the + * operation inline; the real EDT pumps its own loop alongside, which is + * what carries the delivery. The call is kept because it states the + * precondition the assertion below checks -- the operation starts off the + * EDT -- and because it is what a caller would write.

+ * + *

The consequence is that the EDT is free to deliver the moment a + * resource is completed, so every test here has to attach its listener + * before that can happen: an already-settled {@code AsyncResource} runs a + * late listener inline on the attaching thread, recording "not the EDT" + * for a delivery that did happen there. The store tests hold the backend; + * the facade test parks the EDT.

*/ private void assertDeliveredOnEdt(final Landing landing, final Runnable op) { @@ -197,20 +204,6 @@ public void run() { }); } - /** - * The facade's own actions deliver on the EDT too. - * - *

These were missed when the store's results were moved onto - * {@code EdtResult}: {@code openHealthSettings} and - * {@code openProviderSetup} settle synchronously before the method - * returns, so a callback attached afterwards ran immediately on whatever - * thread called -- and a caller doing UI work in it, which is the whole - * point of "did the settings screen open?", raced rendering.

- * - *

The fallback facade is the one under test here because it is the one - * that settles inline; the port facades do the same thing through the - * same resource type.

- */ /** * Every public health resource is an EDT-delivering one. * @@ -310,12 +303,41 @@ private static String enclosingMethod(String[] lines, int at) { return ""; } + /** + * The facade's own actions deliver on the EDT too. + * + *

These were missed when the store's results were moved onto + * {@code EdtResult}: {@code openHealthSettings} and + * {@code openProviderSetup} settle synchronously before the method + * returns, so a callback attached afterwards ran immediately on whatever + * thread called -- and a caller doing UI work in it, which is the whole + * point of "did the settings screen open?", raced rendering.

+ * + *

The fallback facade is the one under test here because it is the one + * that settles inline; the port facades do the same thing through the + * same resource type.

+ */ @Test void aFacadeActionDeliversOnTheEdt() { final Landing landing = new Landing(); + final CountDownLatch attached = new CountDownLatch(1); assertDeliveredOnEdt(landing, new Runnable() { public void run() { + // The facade settles before it returns, so unlike the store + // tests there is nothing to hold back -- park the EDT instead. + // Serial calls run in order, so this blocker is ahead of the + // delivery in the queue and the listener wins the attach. + CN.callSerially(new Runnable() { + public void run() { + try { + attached.await(); + } catch (InterruptedException ex) { + Thread.currentThread().interrupt(); + } + } + }); Health.getInstance().openHealthSettings().onResult(landing); + attached.countDown(); } }, true); } From 165335943a90484b54258aa737f958c0dc1dfb59 Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Tue, 4 Aug 2026 14:29:46 +0700 Subject: [PATCH 2/2] Review: always release the EDT blocker, and bound its wait 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) --- .../codename1/health/HealthEdtDeliveryTest.java | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/maven/core-unittests/src/test/java/com/codename1/health/HealthEdtDeliveryTest.java b/maven/core-unittests/src/test/java/com/codename1/health/HealthEdtDeliveryTest.java index b83bdbda835..5298b549342 100644 --- a/maven/core-unittests/src/test/java/com/codename1/health/HealthEdtDeliveryTest.java +++ b/maven/core-unittests/src/test/java/com/codename1/health/HealthEdtDeliveryTest.java @@ -35,6 +35,7 @@ import java.util.ArrayList; import java.util.List; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import static org.junit.jupiter.api.Assertions.*; @@ -330,14 +331,23 @@ public void run() { CN.callSerially(new Runnable() { public void run() { try { - attached.await(); + // Bounded on purpose: a blocker that outlived its + // release would park the EDT for the rest of the + // suite, turning one failure into a hang. + attached.await(10, TimeUnit.SECONDS); } catch (InterruptedException ex) { Thread.currentThread().interrupt(); } } }); - Health.getInstance().openHealthSettings().onResult(landing); - attached.countDown(); + try { + Health.getInstance().openHealthSettings() + .onResult(landing); + } finally { + // In a finally so a throw above surfaces as itself rather + // than as a suite-wide hang behind a still-parked EDT. + attached.countDown(); + } } }, true); }