Skip to content

Commit

Permalink
Fix Socket telemetry outerloop test failures (#45170)
Browse files Browse the repository at this point in the history
Based purely on code inspection, since I couldn't repro the failures happening in the lab, I believe what's happening is we're not outputting the right events if the connect ends up completing so fast that it's treated as a synchronous completion.  The fix is to move the relevant tracing to be done when the work completes, regardless of the completion mode.

I was able to simulate at least one set of failures by delaying the calling thread before it reaches a particular point, and this fixes that issue, so even if it's not fixing all known problems (hopefully it is), it's at least fixing some.
  • Loading branch information
stephentoub committed Nov 25, 2020
1 parent e08358f commit 0262b49
Showing 1 changed file with 9 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -813,19 +813,25 @@ async Task Core(MultiConnectSocketAsyncEventArgs internalArgs, Task<IPAddress[]>
}

// Complete the operation.
if (SocketsTelemetry.Log.IsEnabled() && !_disableTelemetry) LogBytesTransferEvents(_connectSocket?.SocketType, SocketAsyncOperation.Connect, internalArgs.BytesTransferred);
if (SocketsTelemetry.Log.IsEnabled() && !_disableTelemetry)
{
LogBytesTransferEvents(_connectSocket?.SocketType, SocketAsyncOperation.Connect, internalArgs.BytesTransferred);
AfterConnectAcceptTelemetry();
}
Complete();

// If the caller is treating this operation as pending, own the completion.
// Clean up after our temporary arguments.
internalArgs.Dispose();

// If the caller is treating this operation as pending, own the completion.
if (!internalArgs.ReachedCoordinationPointFirst())
{
// Regardless of _flowExecutionContext, context will have been flown through this async method, as that's part
// of what async methods do. As such, we're already on whatever ExecutionContext is the right one to invoke
// the completion callback. This method may have even mutated the ExecutionContext, in which case for telemetry
// we need those mutations to be surfaced as part of this callback, so that logging performed here sees those
// mutations (e.g. to the current Activity).
OnCompletedInternal();
OnCompleted(this);
}
}
}
Expand Down

0 comments on commit 0262b49

Please sign in to comment.