Branch: refs/heads/master Home: https://github.com/xwiki/xwiki-commons Commit: 6974307afc785333fdb230f4ca6d0e882e892463 https://github.com/xwiki/xwiki-commons/commit/6974307afc785333fdb230f4ca6d0e... Author: Simon Urli <[email protected]> Date: 2026-09-18 (Fri, 18 Sep 2026) Changed paths: M xwiki-commons-core/xwiki-commons-job/xwiki-commons-job-default/src/main/java/org/xwiki/job/internal/ReadWriteSemaphore.java M xwiki-commons-core/xwiki-commons-job/xwiki-commons-job-default/src/test/java/org/xwiki/job/internal/DefaultJobExecutorTest.java A xwiki-commons-core/xwiki-commons-job/xwiki-commons-job-default/src/test/java/org/xwiki/job/internal/ReadWriteSemaphoreTest.java Log Message: ----------- XCOMMONS-2092: Possible race condition in ReadWriteSemaphore (#1963) * ReadWriteSemaphore#lockWrite/unlockWrite/lockRead/unlockRead mutated writeCounter/readCounter with incrementAndGet()/decrementAndGet() and then made the permits decision from a separate, later get() call, letting concurrent callers race and compute the wrong number of permits to acquire or release; a stress test hammering the same parent/child pool shape as the flaky test (pool sizes 1 and 2) reliably deadlocked within a couple hundred operations because of this * Guard the counter mutation and the resulting permit-count decision with an internal lock so each call makes its decision atomically; the lock is released before the (possibly blocking) semaphore acquire/release, so a thread waiting for a permit never holds a lock another thread needs in order to release one * Verified with a stress test reproducing the deadlock against the original code and confirming it cannot reproduce against the fix (5 runs x 3M operations each, no failure), and 500 repetitions of every test in DefaultJobExecutorTest (1500 runs total, 0 failures) * Replace the Semaphore-and-permit-count scheme with a plain monitor (synchronized/wait/notifyAll) that checks readCounter/writeCounter/activeWriters directly: a writer can proceed once activeWriters + readCounter is below the pool size, a reader once no writer is announced or active. This removes the whole class of "permits taken and given back don't match" bugs (the exact one the previous commit's non-atomicity fix targeted, and others found in the course of validating it) by construction: there is nothing to keep resynchronized with reality any more. * Serve waiting writers in the order they called lockWrite(), via an explicit ticket: plain wait()/notifyAll() does not by itself guarantee that a slot freed up for the earliest-still-waiting writer cannot be taken by a later one instead, which matchingGroupPathAreBlockedPoolMultiSizeChildrenFirst caught happening about half the time. * Restore (as a targeted, commented wait, not a blanket workaround) the synchronization the first commit on this issue had removed between submitting a writer and submitting a reader that must queue behind it in matchingGroupPathAreBlockedPoolMultiSizeParentFirst: nothing in ReadWriteSemaphore itself can guarantee an earlier-submitted writer's worker thread reaches its own lockWrite() call before a later-submitted reader's thread reaches lockRead(), since that is a property of when each gets dispatched by the surrounding thread pool, not of the semaphore's internal bookkeeping. * Add ReadWriteSemaphoreTest with a concurrent stress test reproducing, within a few hundred operations, the original non-atomic-counter deadlock this whole change started from. Verified with the concurrency stress test (reliably deadlocks the pre-atomicity-fix code, passes against this one), and with 5000/5000, 5000/5000 and 1500/1500 repeated runs of the three DefaultJobExecutorTest methods (matchingGroupPathAreBlocked, ...ParentFirst, ...ChildrenFirst), each in its own fresh JVM, 0 failures. * java:S2273 on ReadWriteSemaphore#waitUninterruptibly(): both call sites already hold the monitor's lock before calling this private method, but Sonar cannot verify that across the method boundary. Suppressed with a comment explaining why. * java:S2925 on the Thread.sleep() in matchingGroupPathAreBlockedPoolMultiSizeParentFirst: there is no observable state change to poll on instead, as already explained by the comment above the call (and as already accepted, unsuppressed, for the identical pattern in matchingGroupPathAreBlockedPoolMultiSizeChildrenFirst). Suppressed at the test method. * java:S1134 on the same test: dropped the redundant "FIXME:" comment line, since the paragraph above it already explains the constraint in full - it was never an outstanding task, just copied wording from the pre-existing pattern in the sibling test method. * Replace the java:S5961 suppression added by the previous commit with an actual cleanup of DefaultJobExecutorTest. SonarJava counts assertions reached through private helpers, so 10 of the 28 it reported on matchingGroupPathAreBlockedPoolMultiSizeParentFirst were the fail() inside waitJobState(), and 12 of the 18 remaining direct assertions only restated the state the waitJobXxx() call right above them had already established. * Drop those pure restatements in the three matchingGroupPath* tests, keeping every assertNull() (a job that must not have started yet is the property under test) and every "still in that state" re-check no preceding wait covers. Assertion counts as SonarJava computes them: 28 -> 21, 29 -> 21 and 18 -> 12. Splitting the methods instead was rejected: each one is a single choreography over live threads whose assertions are checkpoints on one timeline, so every split test would have to replay all the steps before its own. * Extract the repeated GroupedJobInitializer stubbing into mockPool(poolSize, path...) and mockAllPools(poolSize), so each test opens with the pool sizes its scenario comment describes. * Remove a duplicated assertion on job1 at the end of matchingGroupPathAreBlocked. It was meant to be on job12, whose final state the assertion above it already covers. * Fix two comments mangled by a search and replace ("taken into ABcount", "they are ABtually after"). * waitJobState() reported its timeout through fail(), and assertion-aware analysis resolves helper methods transitively, so every waitJobWaiting()/waitJobFinished() call counted as an assertion of the calling test. Each checkpoint of these tests was therefore counted twice: once for the barrier that drives the choreography to its next step, and once for the assertion that states what is expected there. That is what took matchingGroupPathAreBlockedPoolMultiSizeParentFirst to 28 for a method stating 18 expectations. * Raise that timeout by throwing instead. Assertions.fail() is itself a throw of an AssertionError subclass, so behaviour and Surefire reporting are unchanged, and the count now equals the number of expectations each test states: 28 -> 18, 29 -> 17 and 18 -> 12. The java:S5961 suppression added by the previous commit is dropped. * The same helper resolution applies to "tests should include assertions", where the effect is worse: a test whose only assertion-like call is a wait would have looked like it verifies something. It no longer does. * Restore the interrupt flag and keep the InterruptedException as the cause, instead of swallowing both. * Extract the repeated GroupedJobInitializer stubbing into mockPool(poolSize, path...) and mockAllPools(poolSize), so each test opens with the pool sizes its scenario comment describes. * Fix the last assertion of matchingGroupPathAreBlocked, which was on job1 twice instead of job12 and job1. * Fix two comments mangled by a search and replace ("taken into ABcount", "they are ABtually after"). --------- Co-authored-by: Claude Sonnet 5 <[email protected]> Co-authored-by: Vincent Massol <[email protected]> To unsubscribe from these emails, change your notification settings at https://github.com/xwiki/xwiki-commons/settings/notifications