NetworkChange should capture AsyncLocals (but not to Timer)#26073
NetworkChange should capture AsyncLocals (but not to Timer)#26073stephentoub merged 5 commits intodotnet:masterfrom
Conversation
| } | ||
| if (s_availabilityTimer == null) | ||
| { | ||
| // Don't capture the current ExecutionContext and its AsyncLocals onto the timer causing them to live forever |
There was a problem hiding this comment.
Why not? What's special about this case that the execution context shouldn't flow into the callback? I can see the argument being made in cases where only infrastructure code is invoked in the callback, but here user code is being invoked.
(And if it is necessary here, the current structure of this is problematic, as it could end up throwing an exception if flow is already suppressed.)
There was a problem hiding this comment.
Its capturing the asynclocals for only the first added delegate onto the timer; not the second, third etc; so it would be the wrong locals.
However I assume if the EC was going to be captured its the addition of to the delegate that should capture them (with each addition being its own context)
s_availabilityChangedSubscribers += value;Not the single creation of a globally static timer?
|
it looks ok to me. What is practical behavior change with the new/old code? |
|
The observable behaviour change is if an AsyncLocal with a change event handler was attached it will no longer receive events every time the timer fires https://github.com/dotnet/corefx/issues/25477#issuecomment-346866897 (section "And everything else after here makes no sense" though this is from SqlClient's pool timers) The main issue it addresses is the timer will capture the AsyncLocal values for the first registration for the entire time there is >= 1 events registered (even if the first registration is removed); extending their lifetime and meaning they cannot be GC'd or finalized. It will also restore the first registration's AsyncLocal's onto every subsequent registrations' callbacks. I don't think adding to a delegate captures the specific ExecutionContext for the registration; so if they were depended on (and only ever one registration, when it would work) it could be a problem? (as @stephentoub mentions in #26073 (comment)) |
|
Looking deeper; it looks like the Windows implementation captures the ExecutionContexts individually; then runs the callbacks on them; however the OSX and Linux implementations will only capture the first registration's. Will need modifications to match Windows behaviour |
|
thanks for the explanation @benaadams |
|
Marked as WIP as Linux and OSX need to capture the EC per registration (as per Windows) |
|
@benaadams what is the next step here? |
|
The change is correct; however the event registrations on Linux and OSX should capture the Execution context for each registration and restore the context when firing the event (as per the Windows implementation) |
|
@benaadams do you have plans to finish it off? If you don't have time now, let's close it, until you have :) |
365a1be to
eabb64e
Compare
eabb64e to
cad2f48
Compare
|
@karelz could do with some x-plat deduping; but should be good now |
|
NETFX x86 failures #26615 |
| private static readonly NetworkAvailabilityEventArgs s_notAvailableEventArgs = new NetworkAvailabilityEventArgs(isAvailable: false); | ||
| private static readonly ContextCallback s_runHandlerAvailable = new ContextCallback(RunAvailabilityHandlerAvailable); | ||
| private static readonly ContextCallback s_runHandlerNotAvailable = new ContextCallback(RunAvailabilityHandlerNotAvailable); | ||
| private static readonly ContextCallback s_runAddressChangedHandler = new ContextCallback(RunAddressChangedHandler); |
There was a problem hiding this comment.
This is adding a fair amount of duplication between the Linux and OSX implementations, and it's also missing this optimization for the Windows implementation. Can you move this shared code into a shared NetworkAddressChange.cs partial file, and then also use these s_{not}AvailableEventArgs from the Windows implementation? More can be deduped later, but we should at least avoid adding more duplication.
There was a problem hiding this comment.
Deduped; also copied dictionaries under lock for Linux/OSX and skipped trying to add null listeners as per Windows
| } | ||
|
|
||
| s_addressChangedSubscribers += value; | ||
| s_addressChangedSubscribers.TryAdd(value, ExecutionContext.Capture()); |
There was a problem hiding this comment.
Interesting. This represents a breaking change from the previous implementation, in that previously if you did:
NetworkAddressChangedEventHandler handler = ...;
NetworkAddress.NetworkAddressChanged += handler;
NetworkAddress.NetworkAddressChanged += handler;the handler would then be invoked twice, whereas now it'll only be invoked once. But that "only once" behavior is the same behavior that exists on Windows (both core and netfx), so it makes sense to make this match, and I'm not particularly concerned about this causing any issues, not only because it was already this way on Windows, but because you need to be prepared for any number of these notifications to happen due to the nature of the event.
| case Interop.Sys.NetworkChangeKind.AddressAdded: | ||
| case Interop.Sys.NetworkChangeKind.AddressRemoved: | ||
| s_addressChangedSubscribers?.Invoke(null, EventArgs.Empty); | ||
| foreach (var subscriber in s_addressChangedSubscribers) |
There was a problem hiding this comment.
Nit: var => KeyValuePair<NetworkAddressChangedEventHandler, ExecutionContext>
| if (changed) | ||
| { | ||
| s_availabilityChangedSubscribers?.Invoke(null, new NetworkAvailabilityEventArgs(NetworkInterface.GetIsNetworkAvailable())); | ||
| var isAvailable = NetworkInterface.GetIsNetworkAvailable(); |
|
|
||
| if (ec == null) // Flow supressed | ||
| { | ||
| handler(null, isAvailable ? s_availableEventArgs : s_notAvailableEventArgs); |
There was a problem hiding this comment.
Can you move these isAvailable ? a : b checks to be before the loop, rather than doing it for each invocation?
| { | ||
| s_availabilityChangedSubscribers?.Invoke(null, new NetworkAvailabilityEventArgs(NetworkInterface.GetIsNetworkAvailable())); | ||
| var isAvailable = NetworkInterface.GetIsNetworkAvailable(); | ||
| foreach (var subscriber in s_availabilityChangedSubscribers) |
|
Windows x64 failure (Windows.10.Amd64.Open-Debug-x64) |
|
NETFX x86 Release Build failure |
|
test Windows x64 Debug Build |
|
test NETFX x86 Release Build |
| if (value != null) | ||
| { | ||
| s_addressChangedSubscribers.TryAdd(value, ExecutionContext.Capture()); | ||
| } |
There was a problem hiding this comment.
How about doing the value != null check once, and before taking the lock?
add
{
if (value != null)
{
lock (s_gate)
{
if (s_socket == 0)
{
CreateSocket();
}
s_addressChangedSubscribers.TryAdd(value, ExecutionContext.Capture());
}
}
}| restoreFlow = true; | ||
| } | ||
|
|
||
| s_availabilityTimer = new Timer(s_availabilityTimerFiredCallback, null, -1, -1); |
There was a problem hiding this comment.
Nit: for clarity, can we use Timeout.Infinite instead of -1?
| remove | ||
| { | ||
| lock (s_gate) | ||
| { |
There was a problem hiding this comment.
Do we need to do any of this if value is null? Wondering if there should be parity with add.
| } | ||
| if (s_availabilityTimer == null) | ||
|
|
||
| if (s_availabilityTimer == null && value != null) |
There was a problem hiding this comment.
Some null-related comments as in the other event.
| { | ||
| changed = s_availabilityHasChanged; | ||
| s_availabilityHasChanged = false; | ||
| if (s_availabilityChangedSubscribers.Count > 0) |
There was a problem hiding this comment.
I think this should be:
if (changed && s_availabilityChangedSubscribers.Count > 0)Otherwise we're potentially allocating/copying the dictionary and then not using the copy at all.
And, actually, we don't need changed now. We can just make the condition:
if (s_availabilityHasChanged && s_availabilityChangedSubscribers.Count > 0)and then below the check just becomes:
if (availabilityChangedSubscribers != null)| } | ||
|
|
||
| s_addressChangedSubscribers += value; | ||
| if (value != null) |
There was a problem hiding this comment.
Same comments as earlier about null. I'll stop commenting on these.
|
Done feedback Lets tempt the wrath of an outerloop... @dotnet-bot test Outerloop Windows x64 Debug Build please |
|
All sorts of strange errors... Windows x64 Debug Build |
|
@dotnet-bot test this please |
|
System.IO.Compression.Brotli.Performance.Tests issues across the board it looks like |
|
@dotnet-bot test this please |
…orefx#26073) * NetworkChange shouldn't capture AsyncLocals into its Timer * Capture EC on Linux * Capture EC on OSX * Feedback/dedupe platforms * Better null checking Commit migrated from dotnet/corefx@6000379
Resolves https://github.com/dotnet/corefx/issues/26072