Suppress ExecutionContext while creating timers#6106
Conversation
| // Create the timer, but don't start it; the manual run below will will schedule the first refresh. | ||
| _periodicPasswordProviderTimer = new Timer(state => _ = RefreshPassword(), null, Timeout.InfiniteTimeSpan, Timeout.InfiniteTimeSpan); | ||
| // Create the timer, but don't start it; the manual run below will schedule the first refresh. | ||
| using (ExecutionContext.SuppressFlow()) // Don't capture the current ExecutionContext and its AsyncLocals onto the timer causing them to live forever |
There was a problem hiding this comment.
We might want to extend that so the first refresh attempt behaves the exact same way as the later ones.
|
|
||
| _sendFeedbackTimer = new Timer(TimerSendFeedback, state: null, WalReceiverStatusInterval, Timeout.InfiniteTimeSpan); | ||
| _requestFeedbackTimer = new Timer(TimerRequestFeedback, state: null, _requestFeedbackInterval, Timeout.InfiniteTimeSpan); | ||
| using (ExecutionContext.SuppressFlow()) // Don't capture the current ExecutionContext and its AsyncLocals onto the timer causing them to live forever |
There was a problem hiding this comment.
Strictly speaking, this shouldn't really be necessary since this is IAsyncEnumerator and timers will be disposed whenever the enumerator is, but might as well do this for consistency.
|
It actually needs to look like this for Timer dotnet/runtime#24770 (comment) (suppress throws if it's already suppressed, they're extremely ugly apis) Regardless, it's unfortunate UnsafeCreate never made it into the BCL, might be worth re-opening because all the other linked issues didn't pan out either. |
Hmm, I never knew about this! I actually copy-pasted this from the runtime, so I guess it's also a bug there? |
|
Yep :) |
|
Though looking through the code, didn't dotnet/runtime#82912 fix this? |
|
Huh and I didn't know about that one 🤣 OK then the xmldoc is just outdated. |
vonzshik
force-pushed
the
6105-suppress-execution-context-for-timers
branch
from
e9bad6c to
5255655
Compare
|
@vonzshik I think this can be merged without additional reviewing? |
Probably? Mostly, I just wanted to keep you in the loop since we do to some extent change how |
|
OK, understood. BTW IIRC we discourage the periodic password provider in any case, we could even consider obsoleting it at some point. |
3 participants
Fixes #6105