From deae04a4069d7718b46e0f2fddd39cc76f5227b4 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Tue, 24 Nov 2020 17:01:40 -0500 Subject: [PATCH] Fix Socket telemetry outerloop test failures 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. --- .../src/System/Net/Sockets/SocketAsyncEventArgs.cs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.cs index 89a9bfffb1bc98..bda1217e01f20a 100644 --- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.cs +++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.cs @@ -813,11 +813,17 @@ caughtException is OperationCanceledException || } // 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 @@ -825,7 +831,7 @@ caughtException is OperationCanceledException || // 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); } } }