reiern70 opened a new issue, #1616:
URL: https://github.com/apache/wicket/issues/1616
### Summary
`AsynchronousPageStoreTest.storeAsynchronousContextClosed` passes without
ever
running the assertions it exists to make, and fails intermittently in CI for
an
unrelated reason. It has been hiding a wrong expectation about
`AsynchronousPageStore.PendingAdd#getSessionAttribute`.
### What the test intends
It checks what an `IPageStore` may do with the `IPageContext` it is handed
once
the add has moved to the page saving thread: the session id is still
readable,
request data is not, session data and attributes read back but cannot be
changed.
### Why it does not
```java
asyncPageStore.addPage(context, page);
store.destroy(); // the delegate, not the AsynchronousPageStore
if (asyncFail.get() != null) {
throw asyncFail.get();
}
```
`addPage` queues a `PendingAdd` and returns. `destroy()` is called on the
delegate store, so `AsynchronousPageStore#destroy()` — the method that
interrupts and joins the page saving thread — never runs. The test reads
`asyncFail` and returns while the saving thread has usually not started, so
the
overridden `addPage` holding every assertion is typically never executed.
### Consequence 1: intermittent CI failure
Assertion failures raised on the saving thread are `Error`s, so
`PageAddingRunnable`'s `catch (Exception x)` does not hold them. They escape
onto a daemon thread that outlives the test and is writing into a store the
test already destroyed. Surfaced at whatever point the runner notices, this
appears as:
```
[ERROR] Errors:
[ERROR] AsynchronousPageStoreTest.storeAsynchronousContextClosed »
```
The same test passes on a rerun, which is what makes it read as flaky.
### Consequence 2: a wrong expectation went unnoticed
Once the test actually waits for the add, it fails:
```
Expected org.apache.wicket.WicketRuntimeException to be thrown, but nothing
was thrown.
at
AsynchronousPageStoreTest$6.addPage(AsynchronousPageStoreTest.java:414)
```
The test required this to throw:
```java
context.getSessionAttribute("key2", () -> null);
```
It does not, and should not. `PendingAdd#getSessionAttribute` throws only
where
a value would be *changed*:
```java
value = (T)attributeCache.get(key);
if (value == null && defaultValue.get() != null)
{
throw new WicketRuntimeException("session attribute can not be changed
asynchronuously");
}
```
A read of a missing key with a `null` default changes nothing and returns
`null`. The `getSessionData` block three lines above in the same test already
expects exactly that. The attribute expectation is simply wrong.
**No production behaviour is at fault here** — this is a test defect only.
### Proposed fix
- Count down a `CountDownLatch` in a `finally` around the asynchronous body
and
`assertTrue(added.await(...))`, so an add that never happens fails the test
rather than passing it.
- Record any `Throwable` from that body for the test thread to rethrow. With
failures able to propagate, the sentinel idiom
(`asyncFail.set(new Exception().fillInStackTrace())` followed by swallowing
the expected exception) gives way to `assertThrows`.
- Call `destroy()` on the `AsynchronousPageStore`, which interrupts and joins
the saving thread and still reaches the delegate via `DelegatingPageStore`.
- Correct the attribute expectations to mirror the session-data ones: a
cached
key reads back, a missing key reads `null`, only an attempted set throws.
Verified with `mvn clean verify -Pjs-test` green and 8/8 on repeated runs of
the
class. Branch: `fix-async-pagestore-test`.
### Related
`runTest` in the same class has the same leak — it destroys the delegate
rather
than the `AsynchronousPageStore`, so the other four tests each leak a saving
thread. Not addressed here.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
