Add fork detection for upgradeable read lock context#1594
Open
lifengl wants to merge 1 commit into
Open
Conversation
Add a virtual method OnLockForkDetected() that is called when a nested read lock request is detected from a forked execution context of an upgradeable read lock holder. This parallels the existing write lock fork check in TryIssueLock (line 1110-1113) but is non-breaking: instead of throwing, it calls a virtual method that consumers can override for telemetry/diagnostics. The fork is detected when: - A read lock is requested (!previouslyQueued, first request only) - The nesting chain contains an active upgradeable read lock - The calling thread is MTA (CanCurrentThreadHoldActiveLock) - The calling thread lacks NonConcurrentSynchronizationContext The virtual method is called outside syncObject to avoid reentrancy concerns. This addresses the root cause of Watson crash bug #1761459 in CPS, where IsLockSupportingContext can flip between IsCompleted and OnCompleted when a nesting UR lock is released by another thread. The detection enables CPS (and other consumers) to identify and diagnose fork scenarios via telemetry. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds a diagnostic hook to AsyncReaderWriterLock to detect a specific execution-context fork pattern: requesting a nested read lock while an upgradeable read lock is active in the nesting chain, but the current thread lacks the required NonConcurrentSynchronizationContext. This enables downstream consumers (e.g., CPS) to log telemetry for these fork scenarios without introducing a breaking behavioral change (like throwing).
Changes:
- Added a new
protected virtual void OnLockForkDetected()hook onAsyncReaderWriterLock. - Implemented fork detection for nested read lock requests under an active upgradeable read lock in
TryIssueLock, invoking the hook outsidesyncObject. - Added tests validating detection triggers for upgradeable-read forks, and does not trigger for correct-context nesting or read-only forks.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Microsoft.VisualStudio.Threading/AsyncReaderWriterLock.cs | Adds OnLockForkDetected() and detection logic in TryIssueLock for upgradeable-read forked nested read requests. |
| test/Microsoft.VisualStudio.Threading.Tests/AsyncReaderWriterLockTests.cs | Adds coverage validating detection behavior (trigger + two non-trigger scenarios) via a derived lock that counts callbacks. |
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
Adds a virtual method
OnLockForkDetected()onAsyncReaderWriterLockthat is called when a nested read lock request is detected from a forked execution context of an upgradeable read lock holder. This enables consumers like CPS to override the method and send telemetry to identify fork scenarios.Background
This addresses Watson crash bug #1761459 in CPS (
ProjectLockService.OnCompleted), whereIsLockSupportingContextcan flip betweenIsCompletedandOnCompletedwhen a nesting UR lock is released by another thread. The root cause is a race condition that occurs when code holding an upgradeable read lock usesConfigureAwait(false)and then requests a nested read lock from the forked context.Design
The existing write lock already has a fork check in
TryIssueLock(lines 1110-1113) that throwsInvalidOperationException. For the upgradeable read case, we use a non-breaking approach: a virtual method hook instead of throwing, since throwing would be a behavior change affecting many consumers.Fork detection criteria (all must be true):
!previouslyQueued— first request only, to avoid duplicate telemetry fromPendAwaiterretries)hasUpgradeableRead)!hasWrite— the write fork path already handles that)CanCurrentThreadHoldActiveLock— excludes STA/UI thread)NonConcurrentSynchronizationContextThe virtual method is called outside
syncObjectto avoid reentrancy concerns.Changes
AsyncReaderWriterLock.cs: AddedOnLockForkDetected()virtual method and fork detection logic inTryIssueLockAsyncReaderWriterLockTests.cs: Added 3 tests and aLockWithForkDetectionhelper class:UpgradeableReadLockForksAndAsksForReadLock— verifies detection fires on forked UR contextUpgradeableReadLockDoesNotForkOnCorrectContext— verifies no false positive on correct contextReadLockForkFromReadOnlyContextDoesNotTriggerDetection— verifies no detection for read-only forksTest Results
All 153
AsyncReaderWriterLockTestspass (149 succeeded, 4 pre-existing skips).