You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@wicket.apache.org by "reiern70 (via GitHub)" <gi...@apache.org> on 2026/09/23 18:46:39 UTC
[PR] Make the asynchronous page store context test observe its own assertions [wicket]
reiern70 opened a new pull request, #1617:
URL: https://github.com/apache/wicket/pull/1617
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.
GitHub issue #1616: https://github.com/apache/wicket/issues/1616
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make the asynchronous page store context test observe its own assertions [wicket]
Posted by "reiern70 (via GitHub)" <gi...@apache.org>.
reiern70 commented on code in PR #1617:
URL: https://github.com/apache/wicket/pull/1617#discussion_r4159600027
##########
wicket-core-tests/src/test/java/org/apache/wicket/pageStore/AsynchronousPageStoreTest.java:
##########
@@ -271,7 +273,7 @@ public IManageablePage getPage(IPageContext context, int id)
assertEquals(null, asyncPageStore.getPage(context, pageId));
- store.destroy();
+ asyncPageStore.destroy();
semaphore.release();
Review Comment:
I checked it by running `dontKeepPendingPagesInMapIfSessionExpires` in a loop in a single JVM (JDK 24, 30 s timeout per iteration):
- With the test as you reviewed it (9658da8944), it hangs on the first iteration: the test thread is stuck in `Thread.join` at `AsynchronousPageStore.destroy()`, and the saving thread is back in `queue.poll(...)` in `PageAddingRunnable.run`, the same thing you saw.
- With bce7a1af7b it passed 500 iterations in a row.
The whole `AsynchronousPageStoreTest` also passes under `mvn verify`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make the asynchronous page store context test observe its own assertions [wicket]
Posted by "papegaaij (via GitHub)" <gi...@apache.org>.
papegaaij merged PR #1617:
URL: https://github.com/apache/wicket/pull/1617
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make the asynchronous page store context test observe its own assertions [wicket]
Posted by "papegaaij (via GitHub)" <gi...@apache.org>.
papegaaij commented on code in PR #1617:
URL: https://github.com/apache/wicket/pull/1617#discussion_r4156615340
##########
wicket-core-tests/src/test/java/org/apache/wicket/pageStore/AsynchronousPageStoreTest.java:
##########
@@ -271,7 +273,7 @@ public IManageablePage getPage(IPageContext context, int id)
assertEquals(null, asyncPageStore.getPage(context, pageId));
- store.destroy();
+ asyncPageStore.destroy();
semaphore.release();
Review Comment:
@reiern70 can you wrap this up? With this fix, it should be done and ready to merge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make the asynchronous page store context test observe its own assertions [wicket]
Posted by "reiern70 (via GitHub)" <gi...@apache.org>.
reiern70 commented on code in PR #1617:
URL: https://github.com/apache/wicket/pull/1617#discussion_r4159516706
##########
wicket-core-tests/src/test/java/org/apache/wicket/pageStore/AsynchronousPageStoreTest.java:
##########
@@ -271,7 +273,7 @@ public IManageablePage getPage(IPageContext context, int id)
assertEquals(null, asyncPageStore.getPage(context, pageId));
- store.destroy();
+ asyncPageStore.destroy();
semaphore.release();
Review Comment:
Thanks for tracking that down. Both are in now (bce7a1af7b): `semaphore.release()` comes before `asyncPageStore.destroy()`, and the two delegates that block on the semaphore restore the interrupt in their catch, so a saving thread interrupted inside `addPage` exits instead of going back to `poll()`. The commit message describes it as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make the asynchronous page store context test observe its own assertions [wicket]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1617:
URL: https://github.com/apache/wicket/pull/1617#issuecomment-5800962810
## [Codecov](https://app.codecov.io/gh/apache/wicket/pull/1617?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 61.89%. Comparing base ([`6550afd`](https://app.codecov.io/gh/apache/wicket/commit/6550afd842ff15dc807dce8f549f87dbeb4523b1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`bcd74b0`](https://app.codecov.io/gh/apache/wicket/commit/bcd74b0d473d8c5e0fc0407472f5318f832bdde3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #1617 +/- ##
=========================================
Coverage 61.89% 61.89%
- Complexity 11242 11243 +1
=========================================
Files 1247 1247
Lines 48367 48367
Branches 6788 6788
=========================================
+ Hits 29935 29938 +3
+ Misses 15725 15724 -1
+ Partials 2707 2705 -2
```
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make the asynchronous page store context test observe its own assertions [wicket]
Posted by "papegaaij (via GitHub)" <gi...@apache.org>.
papegaaij commented on code in PR #1617:
URL: https://github.com/apache/wicket/pull/1617#discussion_r4097652363
##########
wicket-core-tests/src/test/java/org/apache/wicket/pageStore/AsynchronousPageStoreTest.java:
##########
@@ -271,7 +273,7 @@ public IManageablePage getPage(IPageContext context, int id)
assertEquals(null, asyncPageStore.getPage(context, pageId));
- store.destroy();
+ asyncPageStore.destroy();
semaphore.release();
Review Comment:
Destroying the `AsynchronousPageStore` before `semaphore.release()` can make this test hang forever. It is a race, which is why CI passed; I reproduced the hang locally, with `main` stuck in `Thread.join` at `AsynchronousPageStore.destroy()`.
When the page saving thread picks up the first add before `removeAllPages` runs, it ends up blocked in the delegate's `semaphore.acquire()`. From there:
1. `destroy()` interrupts the thread, and the delegate's `catch (InterruptedException e) {}` swallows the interrupt, clearing the flag.
2. The `while (!Thread.interrupted())` loop in `PageAddingRunnable` therefore carries on and returns to `queue.poll(...)`, so the thread never exits.
3. `destroy()` calls `join()` without a timeout, so it waits forever and `semaphore.release()` is never reached.
When the saving thread has not yet taken the first add, `removeAllPages` clears both, the interrupt lands in `poll()`, and the thread exits cleanly. That is the path the CI runs happened to take. The class is tagged `SLOW`, so `-Pfast` skips it locally.
Releasing the semaphore first lets the blocked add complete, after which the interrupt reaches the thread in `poll()` and `destroy()` returns:
```suggestion
semaphore.release();
asyncPageStore.destroy();
```
Restoring the interrupt in the delegate's catch (`Thread.currentThread().interrupt();`) would also fix it, and makes the test robust against the same ordering mistake later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org