This is an automated email from the ASF dual-hosted git repository. reiern70 pushed a commit to branch fix-async-pagestore-test-10.x in repository https://gitbox.apache.org/repos/asf/wicket.git
commit b8efc733be04b76e1c7ead76ba89c6212d7bd53e Author: reiern70 <[email protected]> AuthorDate: Wed Sep 23 12:54:56 2026 -0500 Make the asynchronous page store context test observe its own assertions storeAsynchronousContextClosed checks what an IPageStore may do with the IPageContext it is handed once the add has moved to the page saving thread. It queued the add, then called destroy() on the delegate store rather than on the AsynchronousPageStore, so nothing ever interrupted or joined that thread. The test read its failure reference and returned while the saving thread had usually not started, and passed by never looking. That left two problems. Assertion failures raised on the saving thread are Errors, so PageAddingRunnable's catch of Exception does not hold them; they escaped onto a daemon thread that outlived the test, writing into a store the test had already destroyed. Reported at whatever point the runner noticed, this is the intermittent failure seen in CI. And because the assertions were never reached, a wrong expectation went unnoticed: the test required getSessionAttribute("key2", () -> null) to throw asynchronously. It does not, and should not. PendingAdd#getSessionAttribute throws only where a value would be changed - a missing entry whose supplier yields a value. A read of a missing key with a null default changes nothing and returns null, which is what the getSessionData block in the same test already expects. The attribute expectations now mirror it: a cached key reads back, a missing key reads null, and only an attempted set throws. No production behaviour changes. The test now counts down a latch in a finally around the asynchronous body and awaits it, so an add that never happens fails the test instead of passing it. The body records any Throwable for the test thread to rethrow, which lets the sentinel "set a marker exception, then swallow the expected one" idiom give way to assertThrows. destroy() is called on the AsynchronousPageStore, interrupting and joining the saving thread, and still reaches the delegate through DelegatingPageStore. runTest, which drives the other four tests in the class, had both faults too. It discarded the result of its latch await, so a run where the pages were never added passed regardless, and it destroyed the delegate rather than the AsynchronousPageStore, leaving a saving thread behind for each of those tests. It now asserts the await and destroys the facade. The three remaining tests wrap their store in an AsynchronousPageStore and then destroyed the delegate, so each left a saving thread running for the rest of the suite. They destroy the facade as well. GitHub issue #1616: https://github.com/apache/wicket/issues/1616 (cherry picked from commit 9658da8944e7a3068435853a5db234aa6a5b2211) --- .../pageStore/AsynchronousPageStoreTest.java | 86 ++++++++++++---------- 1 file changed, 47 insertions(+), 39 deletions(-) diff --git a/wicket-core-tests/src/test/java/org/apache/wicket/pageStore/AsynchronousPageStoreTest.java b/wicket-core-tests/src/test/java/org/apache/wicket/pageStore/AsynchronousPageStoreTest.java index ffd85e5020..fb56cd507e 100644 --- a/wicket-core-tests/src/test/java/org/apache/wicket/pageStore/AsynchronousPageStoreTest.java +++ b/wicket-core-tests/src/test/java/org/apache/wicket/pageStore/AsynchronousPageStoreTest.java @@ -17,6 +17,8 @@ package org.apache.wicket.pageStore; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; @@ -162,7 +164,7 @@ public class AsynchronousPageStoreTest assertEquals(page, pageBack); - store.destroy(); + asyncPageStore.destroy(); } /** @@ -218,7 +220,7 @@ public class AsynchronousPageStoreTest assertEquals(page, pageBack); - store.destroy(); + asyncPageStore.destroy(); } /** @@ -272,7 +274,7 @@ public class AsynchronousPageStoreTest assertEquals(null, asyncPageStore.getPage(context, pageId)); - store.destroy(); + asyncPageStore.destroy(); semaphore.release(); } @@ -363,7 +365,9 @@ public class AsynchronousPageStoreTest public void storeAsynchronousContextClosed() throws Throwable { final AtomicReference<Throwable> asyncFail = new AtomicReference<>(); - + + final CountDownLatch added = new CountDownLatch(1); + IPageStore store = new MockPageStore() { @Override @@ -387,38 +391,39 @@ public class AsynchronousPageStoreTest @Override public synchronized void addPage(IPageContext context, IManageablePage page) { - // can get session id - context.getSessionId(true); - - // cannot access request - try { - context.getRequestData(KEY1, () -> null); - asyncFail.set(new Exception().fillInStackTrace()); - } catch (WicketRuntimeException expected) { - } - try { - context.getRequestData(KEY2, () -> null); - asyncFail.set(new Exception().fillInStackTrace()); - } catch (WicketRuntimeException expected) { + // an assertion failing here would die with the page saving thread + try + { + // can get session id + context.getSessionId(true); + + // cannot access request + assertThrows(WicketRuntimeException.class, + () -> context.getRequestData(KEY1, () -> null)); + assertThrows(WicketRuntimeException.class, + () -> context.getRequestData(KEY2, () -> null)); + + // can read session data + assertEquals("value1", context.getSessionData(KEY1, () -> "value2")); + assertEquals(null, context.getSessionData(KEY2, () -> null)); + // .. but cannot set + assertThrows(WicketRuntimeException.class, + () -> context.getSessionData(KEY2, () -> "value2")); + + // can read session attribute already read + assertEquals("value1", context.getSessionAttribute("key1", () -> null)); + assertNull(context.getSessionAttribute("key2", () -> null)); + // .. but cannot set + assertThrows(WicketRuntimeException.class, + () -> context.getSessionAttribute("key2", () -> "value2")); } - - // can read session data - assertEquals("value1", context.getSessionData(KEY1, () -> "value2")); - assertEquals(null, context.getSessionData(KEY2, () -> null)); - // .. but cannot set - try { - context.getSessionData(KEY2, () -> "value2"); - asyncFail.set(new Exception().fillInStackTrace()); - } catch (WicketRuntimeException expected) { + catch (Throwable failure) + { + asyncFail.set(failure); } - - // can read session attribute already read - assertEquals("value1", context.getSessionAttribute("key1", () -> null)); - // .. but nothing new - try { - context.getSessionAttribute("key2", () -> null); - asyncFail.set(new Exception().fillInStackTrace()); - } catch (WicketRuntimeException expected) { + finally + { + added.countDown(); } } }; @@ -430,9 +435,11 @@ public class AsynchronousPageStoreTest IPageContext context = new MockPageContext(); asyncPageStore.addPage(context , page); - - store.destroy(); - + + assertTrue(added.await(30, TimeUnit.SECONDS), "page was never added asynchronously"); + + asyncPageStore.destroy(); + if (asyncFail.get() != null) { throw asyncFail.get(); } @@ -525,9 +532,10 @@ public class AsynchronousPageStoreTest } } - lock.await(pages * sessions * (writeMillis + readMillis), TimeUnit.MILLISECONDS); + assertTrue(lock.await(pages * sessions * (writeMillis + readMillis), TimeUnit.MILLISECONDS), + "not all pages were added"); - pageStore.destroy(); + asyncPageStore.destroy(); return results; }
