Fix TransactionManager DefaultTimeout/MaximumTimeout setter race condition - #130664
Merged
Conversation
…t setters The DefaultTimeout setter was using Interlocked.Exchange to write s_defaultTimeoutTicks and then setting s_defaultTimeoutValidated = true without holding a lock. This raced with the getter's LazyInitializer.EnsureInitialized (which uses s_classSyncObject as its lock) - a concurrent getter thread could see s_defaultTimeoutValidated as false, enter EnsureInitialized, acquire the lock, and overwrite the setter's new value with the config default. Fix: wrap the writes in DefaultTimeout.set in lock(ClassSyncObject) so they are mutually exclusive with EnsureInitialized. Keep Interlocked.Exchange for 32-bit atomicity of the 64-bit field. Also fix MaximumTimeout.set which had the same issue: - Wrap writes in lock(ClassSyncObject) - Fix wrong flag-before-value ordering (was: s_cachedMaxTimeout = true then s_maximumTimeout = value; now: value before flag) Fixes #105124 Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix test failure in TransactionManagerTest for default timeout
Fix TransactionManager DefaultTimeout/MaximumTimeout setter race condition
Jul 14, 2026
jkotas
reviewed
Jul 29, 2026
Per reviewer feedback, keep the original comparison style (Interlocked.Read(ref s_defaultTimeoutTicks) != value.Ticks) which checks whether the stored timeout differs from what was requested, rather than checking whether validation changed the value. Move the comparison inside the lock so it reads back the value just stored by this thread (not a concurrent thread's modification). Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Member
|
- Delete MachineSettingsSection, ConfigurationStrings, DefaultSettingsSection files - Remove those files from the .csproj - Initialize s_defaultTimeoutTicks with TimeSpan.FromMinutes(1).Ticks - Rename s_maximumTimeout to s_maximumTimeoutTicks (long), initialize with TimeSpan.FromMinutes(10).Ticks - Delete s_defaultTimeoutValidated and s_cachedMaxTimeout - Simplify DefaultTimeout getter: direct Interlocked.Read + combined ETW MethodEnter/Exit - Simplify MaximumTimeout getter: direct Interlocked.Read + combined ETW MethodEnter/Exit - DefaultTimeout setter: ValidateTimeout inside lock, Interlocked.Exchange, compare validated vs original - MaximumTimeout setter: set max with Interlocked.Exchange, revalidate default, fire event outside lock - Inline "" for DistributedTransactionManagerName Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Contributor
Author
Done in the latest commit. Here's a summary of all changes made:
|
jkotas
marked this pull request as ready for review
August 3, 2026 03:40
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to this area: @SamMonoRT, @dotnet/efteam |
AndriySvyryd
approved these changes
Aug 3, 2026
Member
|
@steveisok Could you please approve this? The approval from @AndriySvyryd is not good enough for some reason. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/libraries/System.Transactions.Local/src/System/Transactions/TransactionManager.cs:353
- Inside MaximumTimeout setter,
s_defaultTimeoutTicksis read directly (new TimeSpan(s_defaultTimeoutTicks)) even though the field is otherwise accessed via Interlocked. While this is currently under the same lock as all writers, usingInterlocked.Readhere keeps the access pattern consistent and avoids relying on that invariant for correctness on 32-bit / future refactors.
TimeSpan timeout = new TimeSpan(s_defaultTimeoutTicks);
steveisok
approved these changes
Aug 3, 2026
1 task
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.
TransactionManager.DefaultTimeouthas a race condition where a concurrent read can overwrite a value set by another thread. The getter usesLazyInitializer.EnsureInitialized(which acquiress_classSyncObjectlock) while the setter usedInterlocked.Exchange+ a plain write to thes_defaultTimeoutValidatedflag — outside any lock. A concurrent getter thread could see the flag asfalse, enterEnsureInitialized, acquire the lock, and overwrite the setter's new value with the config default.Changes
Fixes #105124