Make the locking policy of LockedExecutor and StripeLockedExecutor configurable - #316
Make the locking policy of LockedExecutor and StripeLockedExecutor configurable#316fh-ms wants to merge 3 commits into
Conversation
…nfigurable Both executors created their lock via the no-arg `new ReentrantReadWriteLock()`, which is the non-fair (barging) policy, and no overload allowed a caller to choose otherwise. Under that policy an arriving writer always barges ahead of the queue, and an arriving reader only defers when the node at the head of the queue happens to be a writer, so no acquisition order is guaranteed and a waiting thread can be overtaken an unbounded number of times. The Javadoc never mentioned any of this. Fairness is now an explicit choice via `LockedExecutor.New(boolean)` and `StripeLockedExecutor.New(int, boolean)`. The default stays non-fair, so `New()` and `global()` behave exactly as before and no consumer loses throughput. `LockScope` and `StripeLockScope` gained an overridable `createExecutor()` hook so subclasses can select a policy, mirroring the existing `stripeCount()` hook. The Javadoc now documents the policy, its starvation exposure, that reentrancy holds in both modes, and that upgrading a read lock to a write lock deadlocks. Three defects in `StripeLockedExecutor.Default` are fixed along the way. `abs(mutex.hashCode()) % length` produced a negative stripe index for a mutex whose hash code is `Integer.MIN_VALUE`, since `Math.abs` of that value is itself negative; the index is now derived with `Math.floorMod`. The lock array was allocated in the constructor although its field is transient, so a deserialized instance dereferenced null; allocation moved into the lazy accessor, keyed off a non-transient stripe count. The per-element double-checked initialization relied on the array reference's volatility to publish its elements, which it does not do; the array is now populated completely before it is published. The `mutex` parameter is also checked against null, as the Javadoc already promised. The base module gains its first tests, covering the fairness wiring, reentrancy in both policies, lock release on exceptions, mutual exclusion and stripe independence, and each of the three defects. All test library versions move into the root pom's dependencyManagement so base, integration-tests and test-fixtures share them.
There was a problem hiding this comment.
Pull request overview
This pull request makes the locking fairness policy of LockedExecutor and StripeLockedExecutor configurable (while keeping defaults non-fair), documents the behavioral implications (fairness, reentrancy, and read→write upgrade deadlock), and fixes several correctness issues in StripeLockedExecutor.Default around stripe indexing and safe/lazy initialization across serialization.
Changes:
- Add fair/non-fair configuration via
LockedExecutor.New(boolean)andStripeLockedExecutor.New(int, boolean), plus scope hooks (createExecutor()) for overriding the executor configuration. - Fix stripe selection (
floorModforInteger.MIN_VALUE) and make stripe locks lazily and safely initialized (including after deserialization). - Add a new
basetest suite for concurrency executors/scopes and centralize test dependency versions in the parent POM dependencyManagement.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test-fixtures/pom.xml | Removes per-module JUnit version, relying on parent dependencyManagement. |
| pom.xml | Centralizes test library versions and imports junit-bom in dependencyManagement. |
| integration-tests/pom.xml | Removes local JUnit BOM/version management; relies on parent-managed test dependency versions. |
| base/pom.xml | Adds JUnit Jupiter test dependency and configures surefire to run on classpath for reflection-based tests. |
| base/src/main/java/org/eclipse/serializer/concurrency/LockScope.java | Adds overridable createExecutor() hook to control executor configuration (e.g., fairness). |
| base/src/main/java/org/eclipse/serializer/concurrency/StripeLockScope.java | Adds overridable createExecutor() hook mirroring stripeCount() configurability. |
| base/src/main/java/org/eclipse/serializer/concurrency/LockedExecutor.java | Adds fair/non-fair constructor overload and documents fairness/reentrancy semantics. |
| base/src/main/java/org/eclipse/serializer/concurrency/StripeLockedExecutor.java | Adds fair/non-fair constructor overload, fixes stripe index and safe lazy lock initialization, and documents behavior. |
| base/src/test/java/org/eclipse/serializer/concurrency/LockScopeTest.java | New tests for scope executor wiring and transient re-init behavior. |
| base/src/test/java/org/eclipse/serializer/concurrency/StripeLockScopeTest.java | New tests for stripe scope executor wiring and transient re-init behavior. |
| base/src/test/java/org/eclipse/serializer/concurrency/LockedExecutorTest.java | New tests for fairness wiring, reentrancy, lock release on exceptions, and fairness under load. |
| base/src/test/java/org/eclipse/serializer/concurrency/StripeLockedExecutorTest.java | New tests covering stripe index edge cases, null mutex behavior, transient re-init, and stripe concurrency behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| * A fair executor grants access to the longest-waiting thread and therefore cannot starve any | ||
| * thread, but its throughput is considerably lower because every hand-over has to unpark the next | ||
| * thread instead of letting an already running one barge in. A non-fair executor lets arriving | ||
| * threads barge ahead of waiting ones, which yields a much higher throughput but provides no | ||
| * ordering guarantee at all. |
There was a problem hiding this comment.
Good catch, the claim was too strong. ReentrantReadWriteLock documents fair mode as an approximate arrival-order policy, and ReentrantLock explicitly notes that lock fairness does not imply fairness of thread scheduling.
Reworded in b5089ca: the executor now "hands the lock over in approximate arrival order, so a waiting thread is not overtaken indefinitely", plus an explicit note that a fair policy orders the hand-over of the lock itself and cannot prevent the JVM or the OS from scheduling the contending threads unevenly.
| * A fair executor grants access to the longest-waiting thread and therefore cannot starve any | ||
| * thread, but its throughput is considerably lower because every hand-over has to unpark the next | ||
| * thread instead of letting an already running one barge in. A non-fair executor lets arriving | ||
| * threads barge ahead of waiting ones, which yields a much higher throughput but provides no | ||
| * ordering guarantee at all. |
There was a problem hiding this comment.
Good catch, the claim was too strong. ReentrantReadWriteLock documents fair mode as an approximate arrival-order policy, and ReentrantLock explicitly notes that lock fairness does not imply fairness of thread scheduling.
Reworded in b5089ca: the executor now "hands the lock over in approximate arrival order, so a waiting thread is not overtaken indefinitely", plus an explicit note that a fair policy orders the hand-over of the lock itself and cannot prevent the JVM or the OS from scheduling the contending threads unevenly.
The Javadoc of both `New(boolean)` overloads stated that a fair executor "grants access to the longest-waiting thread and therefore cannot starve any thread". That is stronger than what the underlying lock guarantees: `ReentrantReadWriteLock` documents fair mode as an approximate arrival-order policy, and `ReentrantLock` explicitly notes that the fairness of a lock does not imply fairness of thread scheduling. The wording now says the lock is handed over in approximate arrival order so a waiting thread is not overtaken indefinitely, and adds an explicit note that a fair policy orders the hand-over of the lock itself and cannot influence how the JVM or the operating system schedules the contending threads.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (1)
base/src/main/java/org/eclipse/serializer/concurrency/StripeLockedExecutor.java:248
reentrantLocks()now instantiates aReentrantReadWriteLockfor every stripe the first time the executor is used. This is a behavioral/performance change from the previous per-stripe lazy initialization and can add noticeable allocation/GC overhead when callers pass a largestripeCountbut only touch a small subset of stripes. Consider whether eager initialization is acceptable for your expected stripe counts, or switch to a per-stripe thread-safe lazy approach (e.g.,AtomicReferenceArray+ CAS) if you want to preserve the previous allocation profile.
reentrantLocks = new ReentrantReadWriteLock[this.stripeCount];
for(int i = 0; i < reentrantLocks.length; i++)
{
reentrantLocks[i] = new ReentrantReadWriteLock(this.fair);
}
The lock array is now populated completely before it is published, so all stripes are created the first time the executor is used rather than one by one on demand. That is a deliberate trade for a correctly published array and a hot path without a CAS, but it does mean an oversized stripe count is paid for up front. Say so on `New(int, boolean)` and point at the degree of parallelism as the yardstick for sizing, so the parameter is not mistaken for the number of mutexes the executor will see.
|
Round 2 came back with no new comments. Copilot also raised a suppressed comment about The cost only matters if the stripe count is sized like the number of mutexes rather than like a degree of parallelism, so that is now documented on |
Why
LockedExecutor.DefaultandStripeLockedExecutor.Defaultboth created their lock via the no-argnew ReentrantReadWriteLock(), which is identical tonew ReentrantReadWriteLock(false)- the non-fair (barging) policy - and no overload allowed a caller to choose otherwise.Under that policy
NonfairSync.writerShouldBlock()returnsfalseunconditionally, so an arriving writer always barges ahead of every thread already queued, andreaderShouldBlock()is only theapparentlyFirstQueuedIsExclusive()heuristic over a single racily-read queue node. The result is that no acquisition order is guaranteed and a waiting thread can be overtaken an unbounded number of times. The Javadoc never mentioned the policy at all, so callers had no way to know what they were given.What changed
Fairness is now an explicit, documented choice via
LockedExecutor.New(boolean)andStripeLockedExecutor.New(int, boolean). The default stays non-fair, soNew()andglobal()behave exactly as before and no consumer loses throughput.LockScopeandStripeLockScopegained an overridablecreateExecutor()hook so subclasses can select a policy, mirroring the existingstripeCount()hook.The Javadoc now states the policy, its starvation exposure, that reentrancy holds in both modes, and that upgrading a read lock to a write lock deadlocks - the last one was previously undocumented and is a live foot-gun.
Three defects in
StripeLockedExecutor.Defaultare fixed along the way:abs(mutex.hashCode()) % lengthproduced a negative stripe index for a mutex whose hash code isInteger.MIN_VALUE, becauseMath.absof that value is itself negative. The index is now derived withMath.floorMod.transient, so a deserialized instance dereferenced null. Allocation moved into the lazy accessor, keyed off a non-transient stripe count.The
mutexparameter is also checked against null, as the Javadoc already promised.Tests
The
basemodule gains its first tests (38), covering the fairness wiring, reentrancy in both policies, lock release when an action throws, mutual exclusion and stripe independence, and each of the three defects.Worth knowing:
write_fairExecutorUnderSustainedReadLoad_isNotStarvedis a guarantee test, not a regression test - it would not have failed on the old code. Measured on a 16-core machine, both policies let the writer in within ~25 ms, because the non-fair reader heuristic does its job when readers hold the lock only briefly. The fair/non-fair distinction is not deterministically testable in a unit test, so the actual regression protection comes from the wiring tests and the three defect tests. The read load in that test is real, though: ~1.7M read acquisitions per 200 ms across 16 threads.The stripe-index test deliberately uses a stripe count of 3 rather than a power of two:
Integer.MIN_VALUE % 4 == 0, so a power-of-two stripe count masks the defect entirely. There is also a sweep over stripe counts 1-9 against 13 extreme hash codes.Build
All test library versions (junit-bom, commons-io, javassist, datafaker, slf4j-simple) move into the root pom's
dependencyManagement, sobase,integration-testsandtest-fixturesshare them. Verified withmvn dependency:treebefore and after: no version drift.mvn clean verify -PITis green - 38 tests inbase, 940 inintegration-tests.Compatibility
The public API only grew. No existing signature changed, and
New()/global()still return a non-fair executor.