-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Avoid capturing ExecutionContext into CancellationTokenSource's Timer #18670
Conversation
It's not needed, and it can keep unrelated state alive unnecessarily
| object state, | ||
| int dueTime, | ||
| int period, | ||
| bool flowExecutionContext) |
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.
why you didn't make this internal for now and we can expose it later? why we need to have UnsafeCreate at all?
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.
Just following the proposed API, which maps to things like ThreadPool.QueueUserWorkItem vs ThreadPool.UnsafeQueueUserWorkItem. I don't have a strong preference.
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 am really preferring the constructor over UnsafeCreate. having flowExecutionContext parameter is more useful to understand.
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.
Ok.
|
I added one question, other than that LGTM |
7d75157 to
0072c84
Compare
|
Requested backport to 2.2 https://github.com/dotnet/corefx/issues/31969#issuecomment-416197153 |
…dotnet/coreclr#18670) * Avoid capturing ExecutionContext into CancellationTokenSource's Timer It's not needed, and it can keep unrelated state alive unnecessarily * Address PR feedback Commit migrated from dotnet/coreclr@7d72463
It's not needed, and it can keep unrelated state alive unnecessarily.
Contributes to https://github.com/dotnet/corefx/issues/26523
Contributes to https://github.com/dotnet/corefx/issues/30670
cc: @kouvel, @tarekgh, @benaadams