chore: Add .ConfigureAwait(false) to all async method invocations.
#2177
Conversation
tippmar-nr
requested review from
a team,
jaffinito,
nrcventura,
nr-ahemsath,
chynesNR and
dotnet-agent-team-bot
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2177 +/- ##
==========================================
- Coverage 81.05% 81.04% -0.02%
==========================================
Files 396 396
Lines 24723 24723
Branches 2997 2997
==========================================
- Hits 20040 20036 -4
- Misses 3895 3898 +3
- Partials 788 789 +1
|
| @@ -62,9 +62,9 @@ public string SendData(string method, ConnectionInfo connectionInfo, string seri | |||
| foreach (var header in _requestHeadersMap) | |||
| request.Headers.Add(header.Key, header.Value); | |||
|
|
|||
| using var response = httpClient.SendAsync(request).GetAwaiter().GetResult(); | |||
| using var response = httpClient.SendAsync(request).ConfigureAwait(false).GetAwaiter().GetResult(); | |||
There was a problem hiding this comment.
@nrcventura pointed out (from the doc I linked in the description) that .ConfigureAwait(false).GetAwaiter().GetResult() does exactly nothing different than just .GetAwaiter().GetResult() -- and that was the one area I was really hoping we'd be able to gain some improvement around.
I will note, however, that the .SendAsync() call here is actually calling into NRHttpClient.SendAsync() where the added .ConfigureAwait(false) is actually useful. So, while this change may not do anything beneficial here, it also does no harm.
| @@ -322,10 +322,10 @@ public async Task TryInjectBrowserScriptAsync(string contentType, string request | |||
| if (rumBytes == null) | |||
| { | |||
| transaction.LogFinest("Skipping RUM Injection: No script was available."); | |||
| await baseStream.WriteAsync(buffer, 0, buffer.Length); | |||
| await baseStream.WriteAsync(buffer, 0, buffer.Length).ConfigureAwait(false); | |||
There was a problem hiding this comment.
I don't know if this change is safe or not, because it may rely on data from the synchronization context.
| @@ -25,7 +25,7 @@ public static async Task InjectBrowserScriptAsync(byte[] buffer, Stream baseStre | |||
| { | |||
| // not found, can't inject anything | |||
| transaction?.LogFinest("Skipping RUM Injection: No suitable location found to inject script."); | |||
| await baseStream.WriteAsync(buffer, 0, buffer.Length); | |||
| await baseStream.WriteAsync(buffer, 0, buffer.Length).ConfigureAwait(false); | |||
There was a problem hiding this comment.
I don't know if this stream requires data on the synchronization context.
| @@ -65,7 +65,7 @@ protected void TestConnection() | |||
| wc.DownloadString(testAddress); | |||
| } | |||
| #else | |||
| _lazyHttpClient.Value.GetAsync(testAddress).GetAwaiter().GetResult(); | |||
| _lazyHttpClient.Value.GetAsync(testAddress).ConfigureAwait(false).GetAwaiter().GetResult(); | |||
There was a problem hiding this comment.
This usage of ConfigureAwait does nothing.
| @@ -39,7 +39,7 @@ public async Task Invoke(HttpContext context) | |||
| { | |||
| _agent.Logger.Log(Agent.Extensions.Logging.Level.Finest, "Skipping instrumenting incoming OPTIONS request."); | |||
|
|
|||
| await _next(context); | |||
| await _next(context).ConfigureAwait(false); | |||
There was a problem hiding this comment.
I don't know if this is safe to do. AspNetCore does not have an synchronization context by default, but if one was configured, I don't know if there would be problems with other middleware in the pipeline.
| @@ -77,7 +77,7 @@ public override void Write(byte[] buffer, int offset, int count) | |||
| // Set a flag on the context to indicate we're in the middle of injecting - prevents multiple recursions when response compression is in use | |||
| StartInjecting(); | |||
| _agent.TryInjectBrowserScriptAsync(_context.Response.ContentType, _context.Request.Path.Value, buffer, _baseStream) | |||
| .GetAwaiter().GetResult(); | |||
| .ConfigureAwait(false).GetAwaiter().GetResult(); | |||
There was a problem hiding this comment.
This ConfigureAwait call does nothing.
| @@ -102,7 +102,7 @@ public override async ValueTask WriteAsync(ReadOnlyMemory<byte> buffer, Cancella | |||
| { | |||
| // Set a flag on the context to indicate we're in the middle of injecting - prevents multiple recursions when response compression is in use | |||
| StartInjecting(); | |||
| await _agent.TryInjectBrowserScriptAsync(_context.Response.ContentType, _context.Request.Path.Value, buffer.ToArray(), _baseStream); | |||
| await _agent.TryInjectBrowserScriptAsync(_context.Response.ContentType, _context.Request.Path.Value, buffer.ToArray(), _baseStream).ConfigureAwait(false); | |||
There was a problem hiding this comment.
I'm not sure if this would cause problems if a synchronization was added by someone.
| @@ -39,7 +39,7 @@ public async Task Invoke(HttpContext context) | |||
| { | |||
| _agent.Logger.Log(Agent.Extensions.Logging.Level.Finest, "Not instrumenting incoming OPTIONS request."); | |||
|
|
|||
| await _next(context); | |||
| await _next(context).ConfigureAwait(false); | |||
There was a problem hiding this comment.
Similar to the WrapPipelineMiddleware for older aspnetcore versions, I don't know if this could cause problems if someone added a synchronization context.
| @@ -54,7 +54,7 @@ public async Task Send(ConsumeContext context, IPipe<ConsumeContext> next) | |||
|
|
|||
| var segment = transaction.StartMessageBrokerSegment(mc, MessageBrokerDestinationType.Queue, MessageBrokerAction.Consume, MessageBrokerVendorName, queueData.QueueName); | |||
|
|
|||
| await next.Send(context); | |||
| await next.Send(context).ConfigureAwait(false); | |||
There was a problem hiding this comment.
I don't know if this would cause problems for other filters in the pipeline if there is a synchronization context.
| @@ -55,7 +55,7 @@ public async Task Send(ConsumeContext context, IPipe<ConsumeContext> next) | |||
|
|
|||
| var segment = transaction.StartMessageBrokerSegment(mc, MessageBrokerDestinationType.Queue, MessageBrokerAction.Consume, MessageBrokerVendorName, queueData.QueueName); | |||
|
|
|||
| await next.Send(context); | |||
| await next.Send(context).ConfigureAwait(false); | |||
There was a problem hiding this comment.
Same concern as the non-legacy filter.
| @@ -56,7 +56,7 @@ public override async Task Invoke(IOwinContext context) | |||
|
|
|||
| try | |||
| { | |||
| await _next.Invoke(context); | |||
| await _next.Invoke(context).ConfigureAwait(false); | |||
There was a problem hiding this comment.
I don't know if this would cause problems with other middelwares if a synchronization is present.
Updates all async method invocations to add
.ConfigureAwait(false)which may help prevent deadlocks in certain cases - in particular, we've seen reports of the agent apparently hanging on startup in some instances which may be related to our use ofHttpClient.SendAsync(). Changes are, at worst, harmless and are based on guidance for "library" code from Stephen Toub.Note that unit and integration tests were not updated, as those are "application" code which (generally) shouldn't use
.ConfigureAwait(false).