Skip to content

Fix/thread locking - #195

Merged
cmhulbert merged 6 commits into
developmentfrom
fix/threadLocking
Jan 27, 2026
Merged

Fix/thread locking#195
cmhulbert merged 6 commits into
developmentfrom
fix/threadLocking

Conversation

@cmhulbert

@cmhulbert cmhulbert commented Jan 15, 2026

Copy link
Copy Markdown
Contributor

This PR introduced a ReentrantReadWriteLock based per-thread locking mechanism, backed by a single per-JVM FileLock, to support parallel reads while maintaining exlusive writes through the FileSystemKeyValueAccess

closes #141

@cmhulbert
cmhulbert changed the base branch from master to development January 15, 2026 21:43
@cmhulbert

Copy link
Copy Markdown
Contributor Author

@tpietzsch I added logic to handle cleanup of references, including stale references that were not released. I'll try and describe a bit here.

FileKeyLockManager is a package-private singleton instance now, with static fields for the backing map. when a path is requested with lockFor(Reading/Writing) we store a PhantomReference to that LockedFileChannel. On close, the LockedFileChannel will unlock as expected. if it is never closed, we call clearQueue before each further attempt to lock a new path, which will poll the ReferenceQueue<LockedFileChannel> and for any LockedFileChannel that have since been GCd, we release the underlying KeyLockState. Whether this releases the fileLock, or removed the KeyLockState from the map is dependant on whether there are remainin locks on the KeyLockState still. However, KeyLockState is now package private, so it should not be possible to ever try to lock/unlock except through the lockForReading/Writing methods on the FileSystemKeyValueAccess

@cmhulbert

Copy link
Copy Markdown
Contributor Author

One caveat to the current implementation; we only clearQueue either on an attempt to lockForReading/Writing of a key, or when a LockedFileChannel is closed properly. This means that possibly if the last LockedFileChannel was not closed, and there are no future calls to lockForReading/lockForWriting then the JVM will retain that file lock. There isn't a great way to solve this. The only guaranteed way would be to occasionally poll the ReferenceQueue in another thread, and clear when it is GCd. That would introduce a daemon thread to n5 core though, which I think is not ideal.

Again, all of this is only to handle the case where a declared AutoCloseable is not closed. I think it's worth being defensive to some degree, but the FileKeyLockManager should already gaurantee that stale locks are only ever a consequence of a developer ignoring the AutoCloseable contract

bogovicj and others added 4 commits January 24, 2026 00:57
* multiple read locks can be held across threads
* only a single write lock can be held
* still to do: holding a read lock does not prevent writing

see #141
refactor: FileKeyLockManager is a static singleton now, and more KeyLockState logic is package-private
Signed-off-by: Caleb Hulbert <cmhulbert@gmail.com>
Locks are not tied to the locking thread anymore. (A LockedChannel can be
created by one thread and then be released by another thread). This required to
replace ReentrantReadWriteLock with manual reader/writer counting guarded by
Semaphores.

LockedFileChannel is now created by KeyLockState (instead of FileKeyLockManager)
because for releasing we need to distinguish between read and write locks.

I revised clean-up of stale KeyLockStates by putting WeakReferences in the
ConcurrentHashMap and adding a ReferenceQueue to remove entries whose
KeyLockState has been GCed.

I put no special logic to handle leaked LockedFileChannels (that are abandoned
without being properly closed). Surprisingly, this still works. When the
KeyLockState is GCed, its associated ChannelLock along with the existing
FileChannel and FileLock is also GCed. At least on MacOS, this causes the JVM to
release the system-level lock, and everything works out fine.
@tpietzsch

Copy link
Copy Markdown
Collaborator

I revised locking so that locks are not tied to the locking thread. (A LockedChannel can be created by one thread and then be released by another thread). This required to replace ReentrantReadWriteLock by manual reader/writer counting guarded by Semaphores.

I revised clean-up of stale KeyLockStates by putting WeakReferences in the ConcurrentHashMap and adding a ReferenceQueue to remove entries whose KeyLockState has been GCed.

I put no special logic to handle leaked LockedFileChannels (that are abandoned without being properly closed). Surprisingly, this still works. When the KeyLockState is GCed, its associated ChannelLock along with the existing FileChannel and FileLock is also GCed. At least on MacOS, this causes the JVM to release the system-level lock, and everything works out fine.

@tpietzsch
tpietzsch force-pushed the fix/threadLocking branch 2 times, most recently from 2d3b26b to 8a8975d Compare January 26, 2026 11:05
…nnel

This fixes behaviour on Windows, where the we cannot open another
FileChannel on the write-locked path. We have to use the channel that we
locked.
@cmhulbert
cmhulbert merged commit 7b68e47 into development Jan 27, 2026
2 checks passed
@tpietzsch
tpietzsch deleted the fix/threadLocking branch June 11, 2026 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FileSystemKeyValueAccess doesn't support parallel read access within the same JVM

3 participants