Repository navigation
Add timed read acquisition and queue diagnostics to GraphLock - #294
Merged
mbastian merged 2 commits intoOct 25, 2026
Merged
Conversation
MattArtzAnthro
force-pushed
the
graphlock-timed-acquisition
branch
2 times, most recently
from
August 28, 2026 19:43
ca07fe7 to
890e9e7
Compare
6 of 7 tasks
GraphLock exposed only unbounded, non-interruptible lock() calls, so a caller that cannot afford to wait indefinitely had no option in the public API. This adds tryReadLock and tryWriteLock with a timeout, plus getReadLockCount, isWriteLocked, and getQueueLength for monitoring, each delegating to the underlying ReentrantReadWriteLock. New methods are default methods on the interface so existing implementations keep compiling. Javadoc on readLock now states the consequence of holding a read lock across a cross-thread wait or abandoning an auto-locking iterator. Fixes gephi#282.
MattArtzAnthro
force-pushed
the
graphlock-timed-acquisition
branch
from
September 25, 2026 13:39
890e9e7 to
1ceca12
Compare
- Drop tryWriteLock for now: a queued timed writer stalls every new reader for as long as it waits, and retrying with a short timeout can starve the writer. The need for it is yet to be assessed. - Make the new GraphLock methods abstract, GraphLockImpl is the only implementation, so callers don't have to handle UnsupportedOperationException. - Document that a zero timeout tryReadLock fails behind a queued writer only for threads that don't already hold the lock, that a pending interrupt throws on entry, that nothing may touch the graph after a failed attempt, and that isWriteLocked is for monitoring only. - Test that a queued writer blocks a new tryReadLock, that an interrupted tryReadLock leaves no waiter, and that a holder's zero timeout tryReadLock still succeeds. - Make the new tests wait for the thread to be parked and not leave threads behind on failure, as well as the queue length test. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
mbastian
changed the title
Member
|
Pushed d684bad on top: drops |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GraphLockexposed only unbounded, non-interruptiblereadLock()/writeLock()calls, so a caller that cannot afford to wait indefinitely (a plugin doing graph work off the EDT while the viz engine renders) had no option in the public API short of reflecting intoGraphLockImpl.tryReadLock(timeout, unit), delegating to the underlyingReentrantReadWriteLock's timedtryLock. The timed form enqueues rather than barging, which is why it did not starve in the reproducer on raphLock has no timed/try acquisition; polling writers can starve, and a stalled reader plus a queued writer can wedge all graph operations #282 where untimedtryLock()polling did (0 of 4637 attempts vs 2116 of 2116). A zero timeout returns immediately, and fails behind a queued writer unless the caller already holds the lock.tryWriteLockis left out for now: a queued timed writer stalls every new reader for as long as it waits, and retrying with a short timeout can starve the writer. Its need is still to be assessed.getReadLockCount(),isWriteLocked(), andgetQueueLength()for monitoring.getReadLockCount()counts holds across all threads, which is what makes a leaked read hold (an abandoned auto-locking iterator) diagnosable; today it is invisible to a thread dump once the holding thread has exited.GraphLockImplis the only implementation in this repository. External implementations ofGraphLockwill need to implement them.readLock()now states that a read hold across a cross-thread wait, or an iterator abandoned before exhaustion ordoBreak(), blocks all graph operations once a writer queues. No existing behavior changes; fairness is untouched.Part of #282
Test plan
GraphLockImplTest: timed read acquire when free and after another thread's write is released; a queued writer blocks a newtryReadLock, but not a holder's; interruption during a timed wait propagates and leaves no waiter;getQueueLengthreports a queued writer;getReadLockCountcounts holds from two threads whilegetReadHoldCountstays per-thread;isWriteLockedfollows lock and unlock. Contention is set up with latches, not sleeps, apart from a bounded poll until the contending thread is parked.mvn testpasses the full suite (1657 tests).As with the issue, this was worked out together with Claude, Anthropic's AI assistant; I built and ran the tests on my machine.