Skip to content

Use notifyAll in IOTaskRunnable.kick (CodeQL java/notify-instead-of-notify-all)#243

Open
vharseko wants to merge 3 commits into
OpenIdentityPlatform:masterfrom
vharseko:features/codeql-notify-all
Open

Use notifyAll in IOTaskRunnable.kick (CodeQL java/notify-instead-of-notify-all)#243
vharseko wants to merge 3 commits into
OpenIdentityPlatform:masterfrom
vharseko:features/codeql-notify-all

Conversation

@vharseko

@vharseko vharseko commented Jul 8, 2026

Copy link
Copy Markdown
Member

Summary

Resolves the java/notify-instead-of-notify-all alert in IOTaskRunnable.

kick() woke a blocked worker with notify():

synchronized void kick() {
    if (!_notified) {
        _notified = true;
        notify();
    }
}

Each IOTaskRunnable has exactly one background thread (created in start()),
and that thread is the only one that ever calls wait() on the monitor (in
run()), so notify() and notifyAll() are functionally equivalent today.
However, notify() is fragile — if any other thread ever waited on the same
monitor, it could wake the wrong one and leave the worker blocked.

Fix

Use notifyAll(). With a single waiter it wakes exactly the same thread (no
thundering herd), and the wait loop already re-checks its guards
(shouldStop() / _notified / recomputed wait time), so additional wake-ups are
handled correctly.

Testing

mvn -o -pl persistit/core compile — BUILD SUCCESS.

…otify-all)

kick() woke a waiter with notify(). Today there is only ever a single waiter -
the one background thread per IOTaskRunnable that blocks in run()'s wait() - so
notify() and notifyAll() are equivalent, but notify() is fragile: if another
thread ever waited on the same monitor, notify() could wake the wrong one.

Switch to notifyAll(). It wakes the same single waiter now (no thundering herd),
and the wait loop already re-checks its conditions (shouldStop / _notified /
recomputed wait time), so any extra wake-ups are handled safely.
@vharseko vharseko requested a review from maximthomas July 8, 2026 14:42
@vharseko vharseko added bug codeql CodeQL static-analysis findings labels Jul 8, 2026
vharseko added 2 commits July 9, 2026 12:03
…order

WarmupTest.testWarmup asserted that after a bufferinventory+bufferpreload
restart every buffer slot holds the same page as before shutdown
(getBufferCopy(i) before == getBufferCopy(i) after). That is not what warmup
guarantees: preloadBufferInventory() sorts the recorded pages by read order
(PageNode.READ_COMPARATOR) and reallocates buffers via the clock algorithm, so
a page is reloaded into the pool but not necessarily into its original slot.
The per-slot check only happened to pass when the allocation order coincided,
and flaked on ubuntu-latest / JDK 17 (expected:<0> but was:<2> - an empty slot
where the original held page 2) while passing on JDK 11/21/25/26.

Assert the real invariant instead: every valid, recordable page (skipping
temporary and lock volumes, matching recordBufferInventory) resident at
shutdown is resident again after warmup, comparing the set of pages rather
than their slot positions. A genuine failure to preload a page is still caught.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug codeql CodeQL static-analysis findings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants