-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix 10 minutes time out of TransactionScope #21102
Conversation
@dsplaisted could you have a look? |
SetTransactionManagerField("s_cachedMaxTimeout", true); | ||
SetTransactionManagerField("s_maximumTimeout", timeout); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are static fields, so it probably makes sense to just set them once rather than each time we create a transaction.
0493702
to
9f91479
Compare
@mconnew thanks for the suggestion! Please help me double check the fix |
9f91479
to
b888bd0
Compare
EnlistmentOptions.None); | ||
using (var scope = new TransactionScope( | ||
TransactionScopeOption.Required, | ||
TimeSpan.Zero)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure you want an infinite timeout? A really high timeout will still eventually timeout whereas something going wrong with an infinite timeout can cause things to block indefinitely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the other operations happening within the transaction have their own timeouts, so it wouldn't end up being effectively infinite.
How does one test this PR? I can test locally here and make sure we got it :) |
Build the repo locally, then run |
@mattleibow I tested it, it does work. That's how I test, after running dogfood build, get an VPN (I used my home machine). Find the furthest node so the download would need more than 10 minutes. Run |
@mattleibow let me know when it is good. I do need to get this in soon. |
Talked to Matthew offline. This is good to merge for now |
#21101
Fix #19739