AdvisoryLock: never strand a handle acquired after DisposeAsync drained (#396) - #397
Merged
Merged
Conversation
TryAttainLockAsync checked _disposed only at method entry and then stored the winning handle unconditionally. An acquire in flight when DisposeAsync drained _handles put its handle into the drained dictionary, where nothing would ever dispose it: a granted advisory lock held for the life of the process. With transaction-scoped locks (Marten's default) that is a permanently 'idle in transaction' backend on pg_try_advisory_xact_lock, which Marten's high-water gap detection reads as a live pre-gap reserver and never advances past — JasperFx/marten#5090. The connection is leased by the handle, so ClearAllPools() cannot reclaim it either. The store now happens under the same lock DisposeAsync latches and drains under, which makes the two orderings exhaustive: the handle lands before the drain and is disposed by it, or it observes the disposal and disposes itself. A displaced handle (a lock lost in monitored mode and re-attained) is disposed rather than overwritten in place, and every other _handles access — HasLock, ReleaseLockAsync — moves under the same lock, since the dictionary was being mutated concurrently by the caller's poll loop and by disposal with no synchronization at all. Also adopts JasperFx 2.36.3, which carries the companion fix for the coordinator loop that opens this window (jasperfx#592). Regression test races an acquire against DisposeAsync 20 times and then proves a separate lock can still take the key. Verified RED before the fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Closes #396.
TryAttainLockAsyncchecked_disposedonly at method entry and then stored the winning handle unconditionally:An acquire in flight when
DisposeAsyncdrains puts its handle into the drained dictionary, where nothing will ever dispose it — a granted advisory lock held until the process exits. WithTransactionalLockEnabled = true(Marten's defaultEvents.UseAdvisoryLockTransaction) that is a permanentlyidle in transactionbackend onpg_try_advisory_xact_lock, which Marten's high-water gap detection reads as a live pre-gap reserver and never advances past — JasperFx/marten#5090. The connection is leased by the handle, soNpgsqlConnection.ClearAllPools()cannot reclaim it, and disposing theNpgsqlDataSourceonly does when the consumer owns it.Measured before the fix, sweeping the dispose timing across the whole acquire, 120 iterations each:
DisposeAsyncracing an in-flight acquireSo the shutdown token was never the leak path; the unsynchronized handle store was.
Change
The store now happens under the same lock
DisposeAsynclatches and drains under, which makes the two orderings exhaustive: either the handle lands before the drain and the drain disposes it, or it observes the disposal and disposes itself (reporting not-attained). Two things fall out of that:_handlesaccess moves under the same lock. The dictionary was being mutated concurrently by the caller's leadership poll,ReleaseLockAsync, andDisposeAsyncwith no synchronization at all, and read unsynchronized byHasLock.Adopts JasperFx 2.36.3 (from 2.24.1 / JasperFx.Events 2.0.0), which carries the companion fix for the coordinator loop that opens this window in the first place: JasperFx/jasperfx#592, where
ProjectionCoordinatorBase.StartAsyncdisposed the oldCancellationTokenSourcewithout cancelling it and orphaned a live leadership loop.Tests
an_acquire_that_completes_after_disposal_does_not_strand_its_lock— races an acquire againstDisposeAsync20 times, then proves a completely separateAdvisoryLockcan still take the key. Verified RED before the fix.disposal_releases_a_lock_attained_before_it— the ordinary drain path, andHasLockafter disposal.Local:
Weasel.Postgresql.Tests785 passed / 0 failed / 3 skipped on net10.0.🤖 Generated with Claude Code