Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Refactor to eliminate usage of .GetAwaiter().GetResult() in Framework builds. (#2534) #2535

Merged
merged 2 commits into from
Jun 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2020 New Relic, Inc. All rights reserved.
// Copyright 2020 New Relic, Inc. All rights reserved.
// SPDX-License-Identifier: Apache-2.0

using System;
Expand All @@ -23,7 +23,7 @@ protected HttpClientBase(IWebProxy proxy)
_proxy = proxy;
}

public abstract Task<IHttpResponse> SendAsync(IHttpRequest request);
public abstract IHttpResponse Send(IHttpRequest request);

public virtual void Dispose()
{
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
// Copyright 2020 New Relic, Inc. All rights reserved.
// Copyright 2020 New Relic, Inc. All rights reserved.
// SPDX-License-Identifier: Apache-2.0

#if !NETFRAMEWORK
using System.IO;
using System.Net.Http;
using System.Threading.Tasks;
using NewRelic.Agent.Core.DataTransport.Client.Interfaces;

namespace NewRelic.Agent.Core.DataTransport.Client
Expand All @@ -21,9 +20,9 @@ public HttpContentWrapper(HttpContent httpContent)
_httpContent = httpContent;
}

public Task<Stream> ReadAsStreamAsync()
public Stream ReadAsStream()
{
return _httpContent.ReadAsStreamAsync();
return _httpContent.ReadAsStreamAsync().GetAwaiter().GetResult();
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @tippmar-nr, @nrcventura

Can I ask you how these changes prevent potential deadlocks since I still see GetAwaiter().GetResult() in many places?

Also I'm curious now when you replaced asynchronous calls by sync analogues how this affected performance (e,g. scalability) ?

I'm not familiar with the code base but maybe it was possible to use async natively all the way around?

Thanks

P.S.
I guess the PR description is a bit misleading
"With this change, there are now no usages of .GetAwaiter().GetResult() in code built for the .NET Framework target."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oleksandrk-adorama Thanks for your comment.

The code snippet you referenced above is only included in the .NET Standard 2.0 build of the agent (see line 4).

So while we do still have several usages of .GetAwaiter().GetResult(), none of them occur in the .NET Framework build.

As for performance, there shouldn't be any impact, as the previous calls were synchronously waiting for the async method to finish and have been replaced with direct sync methods.

There are a few architectural concerns with the present implementation of the .NET agent that prevent us from using async everywhere; we're hoping to resolve those concerns in a future version of the agent but have no firm timeline for it at present.


public IHttpContentHeadersWrapper Headers => new HttpContentHeadersWrapper(_httpContent.Headers);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2020 New Relic, Inc. All rights reserved.
// Copyright 2020 New Relic, Inc. All rights reserved.
// SPDX-License-Identifier: Apache-2.0

#if !NETFRAMEWORK
Expand Down Expand Up @@ -28,7 +28,7 @@ public HttpResponse(Guid requestGuid, IHttpResponseMessageWrapper httpResponseMe
_httpResponseMessageWrapper = httpResponseMessageWrapper;
}

public async Task<string> GetContentAsync()
public string GetContent()
{
try
{
Expand All @@ -37,7 +37,7 @@ public async Task<string> GetContentAsync()
return Constants.EmptyResponseBody;
}

var responseStream = await _httpResponseMessageWrapper.Content.ReadAsStreamAsync();
var responseStream = _httpResponseMessageWrapper.Content.ReadAsStream();

var contentTypeEncoding = _httpResponseMessageWrapper.Content.Headers.ContentEncoding;
if (contentTypeEncoding.Contains("gzip"))
Expand All @@ -48,7 +48,7 @@ public async Task<string> GetContentAsync()
using (responseStream)
using (var reader = new StreamReader(responseStream, Encoding.UTF8))
{
var responseBody = await reader.ReadLineAsync();
var responseBody = reader.ReadLineAsync().GetAwaiter().GetResult();

if (responseBody != null)
{
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2020 New Relic, Inc. All rights reserved.
// Copyright 2020 New Relic, Inc. All rights reserved.
// SPDX-License-Identifier: Apache-2.0

using System;
Expand All @@ -8,6 +8,6 @@ namespace NewRelic.Agent.Core.DataTransport.Client.Interfaces
{
public interface IHttpClient : IDisposable
{
Task<IHttpResponse> SendAsync(IHttpRequest request);
IHttpResponse Send(IHttpRequest request);
}
}
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
// Copyright 2020 New Relic, Inc. All rights reserved.
// Copyright 2020 New Relic, Inc. All rights reserved.
// SPDX-License-Identifier: Apache-2.0

using System.IO;
using System.Threading.Tasks;

namespace NewRelic.Agent.Core.DataTransport.Client.Interfaces
{
Expand All @@ -12,7 +11,7 @@ namespace NewRelic.Agent.Core.DataTransport.Client.Interfaces
/// </summary>
public interface IHttpContentWrapper
{
Task<Stream> ReadAsStreamAsync();
Stream ReadAsStream();
IHttpContentHeadersWrapper Headers { get; }
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2020 New Relic, Inc. All rights reserved.
// Copyright 2020 New Relic, Inc. All rights reserved.
// SPDX-License-Identifier: Apache-2.0

using System;
Expand All @@ -11,6 +11,6 @@ public interface IHttpResponse : IDisposable
{
bool IsSuccessStatusCode { get; }
HttpStatusCode StatusCode { get; }
Task<string> GetContentAsync();
string GetContent();
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2020 New Relic, Inc. All rights reserved.
// Copyright 2020 New Relic, Inc. All rights reserved.
// SPDX-License-Identifier: Apache-2.0

#if !NETFRAMEWORK
Expand Down Expand Up @@ -32,7 +32,7 @@ public NRHttpClient(IWebProxy proxy, IConfiguration configuration) : base(proxy)
_httpClientWrapper = new HttpClientWrapper(httpClient, (int)configuration.CollectorTimeout);
}

public override async Task<IHttpResponse> SendAsync(IHttpRequest request)
public override IHttpResponse Send(IHttpRequest request)
{
try
{
Expand Down Expand Up @@ -65,7 +65,7 @@ public override async Task<IHttpResponse> SendAsync(IHttpRequest request)
req.Content.Headers.Add(contentHeader.Key, contentHeader.Value);
}

var response = await _httpClientWrapper.SendAsync(req);
var response = _httpClientWrapper.SendAsync(req).GetAwaiter().GetResult();

var httpResponse = new HttpResponse(request.RequestGuid, response);
return httpResponse;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2020 New Relic, Inc. All rights reserved.
// Copyright 2020 New Relic, Inc. All rights reserved.
// SPDX-License-Identifier: Apache-2.0

#if NETFRAMEWORK
Expand All @@ -25,7 +25,7 @@ public NRWebRequestClient(IWebProxy proxy, IConfiguration configuration) : base(
_configuration = configuration;
}

public override async Task<IHttpResponse> SendAsync(IHttpRequest request)
public override IHttpResponse Send(IHttpRequest request)
{
try
{
Expand Down Expand Up @@ -61,12 +61,10 @@ public override async Task<IHttpResponse> SendAsync(IHttpRequest request)
throw new NullReferenceException("outputStream");
}

// .ConfigureAwait(false) is required here for some reason
await outputStream.WriteAsync(request.Content.PayloadBytes, 0, (int)_httpWebRequest.ContentLength).ConfigureAwait(false);
outputStream.Write(request.Content.PayloadBytes, 0, (int)_httpWebRequest.ContentLength);
}

// .ConfigureAwait(false) is required here for some reason
var resp = (HttpWebResponse)await _httpWebRequest.GetResponseAsync().ConfigureAwait(false);
var resp = (HttpWebResponse)_httpWebRequest.GetResponse();

return new WebRequestClientResponse(request.RequestGuid, resp);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2020 New Relic, Inc. All rights reserved.
// Copyright 2020 New Relic, Inc. All rights reserved.
// SPDX-License-Identifier: Apache-2.0
#if NETFRAMEWORK
using System;
Expand Down Expand Up @@ -26,7 +26,7 @@ public WebRequestClientResponse(Guid requestGuid, HttpWebResponse response)
_response = response;
}

public Task<string> GetContentAsync()
public string GetContent()
{
try
{
Expand All @@ -51,14 +51,14 @@ public Task<string> GetContentAsync()
using (var reader = new StreamReader(responseStream, Encoding.UTF8))
{
var responseBody = reader.ReadLine();
return Task.FromResult(responseBody ?? Constants.EmptyResponseBody);
return responseBody ?? Constants.EmptyResponseBody;
}
}
catch (Exception ex)
{
Log.Error(ex, "Request({0}): Unable to parse response body.", _requestGuid);

return Task.FromResult(Constants.EmptyResponseBody);
return Constants.EmptyResponseBody;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.Send(request);

var responseContent = response.GetContentAsync().GetAwaiter().GetResult();
var responseContent = response.GetContent();

if (!response.IsSuccessStatusCode)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,12 @@
#if !NETFRAMEWORK
using System;
using System.Collections.Generic;
using System.Net.Http;
using System.IO;
using System.IO.Compression;
using Telerik.JustMock;
using NUnit.Framework;
using System.Net;
using System.Text;
using System.Threading.Tasks;
using NewRelic.Agent.Core.DataTransport.Client.Interfaces;
using Telerik.JustMock.Helpers;

Expand Down Expand Up @@ -40,30 +38,30 @@ public void TearDown()
}

[Test]
public async Task GetContentAsync_ReturnsEmptyResponseBody_WhenContentIsNull()
public void GetContent_ReturnsEmptyResponseBody_WhenContentIsNull()
{
_mockHttpResponseMessage.Arrange(message => message.Content).Returns((IHttpContentWrapper)null);

var result = await _httpResponse.GetContentAsync();
var result = _httpResponse.GetContent();

Assert.That(result, Is.EqualTo(Constants.EmptyResponseBody));
}

[Test]
public async Task GetContentAsync_ReturnsContent_WhenContentIsNotNull()
public void GetContent_ReturnsContent_WhenContentIsNotNull()
{
var mockContent = Mock.Create<IHttpContentWrapper>();
var stream = new MemoryStream(Encoding.UTF8.GetBytes(TestResponseBody));
_mockHttpResponseMessage.Arrange(message => message.Content).Returns(mockContent);
mockContent.Arrange(content => content.ReadAsStreamAsync()).ReturnsAsync((Stream)stream);
mockContent.Arrange(content => content.ReadAsStream()).Returns(stream);

var result = await _httpResponse.GetContentAsync();
var result = _httpResponse.GetContent();

Assert.That(result, Is.EqualTo(TestResponseBody));
}

[Test]
public async Task GetContentAsync_HandlesGzipDecompression_WhenContentEncodingIsGzip()
public void GetContent_HandlesGzipDecompression_WhenContentEncodingIsGzip()
{
var compressedStream = new MemoryStream();
using (var gzipStream = new GZipStream(compressedStream, CompressionMode.Compress, true))
Expand All @@ -79,9 +77,9 @@ public async Task GetContentAsync_HandlesGzipDecompression_WhenContentEncodingIs
var mockHeaders = Mock.Create<IHttpContentHeadersWrapper>();
mockContent.Arrange(content => content.Headers).Returns(mockHeaders);
mockHeaders.Arrange(headers => headers.ContentEncoding).Returns(new List<string> { "gzip" });
mockContent.Arrange(content => content.ReadAsStreamAsync()).ReturnsAsync((Stream)compressedStream);
mockContent.Arrange(content => content.ReadAsStream()).Returns(compressedStream);

var result = await _httpResponse.GetContentAsync();
var result = _httpResponse.GetContent();

Assert.That(result, Is.EqualTo(TestResponseBody));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public void TearDown()
_mockHttpClientWrapper.Dispose();
}
[Test]
public async Task SendAsync_ReturnsResponse_WhenSendAsyncSucceeds()
public void Send_ReturnsResponse_WhenSendAsyncSucceeds()
{
// Arrange
var request = CreateHttpRequest();
Expand All @@ -64,7 +64,7 @@ public async Task SendAsync_ReturnsResponse_WhenSendAsyncSucceeds()
.ReturnsAsync(mockHttpResponseMessage);

// Act
var response = await _client.SendAsync(request);
var response = _client.Send(request);

// Assert
Assert.That(response, Is.Not.Null);
Expand All @@ -80,7 +80,7 @@ public void SendAsync_ThrowsHttpRequestException_WhenSendAsyncThrows()
.Throws<HttpRequestException>();

// Act & Assert
Assert.ThrowsAsync<HttpRequestException>(() => _client.SendAsync(request));
Assert.Throws<HttpRequestException>(() => _client.Send(request));
}

private IHttpRequest CreateHttpRequest()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,10 @@
using System;
using System.IO;
using System.Net;
using System.Threading.Tasks;
using NewRelic.Agent.Configuration;
using NewRelic.Agent.Core.DataTransport.Client.Interfaces;
using NUnit.Framework;
using Telerik.JustMock;
using Telerik.JustMock.AutoMock.Ninject.Activation;
using Telerik.JustMock.Helpers;

namespace NewRelic.Agent.Core.DataTransport.Client
{
Expand Down Expand Up @@ -52,20 +49,20 @@ public void TearDown()
}

[Test]
public async Task SendAsync_ShouldReturnValidResponse_WhenWebRequestIsSuccessful()
public void Send_ShouldReturnValidResponse_WhenWebRequestIsSuccessful()
{
// Arrange
var fakeResponse = Mock.Create<HttpWebResponse>();
_client.SetHttpWebRequestFunc(uri =>
{
var mockWebRequest = Mock.Create<HttpWebRequest>();
Mock.Arrange(() => mockWebRequest.GetRequestStream()).Returns(new MemoryStream());
Mock.Arrange(() => mockWebRequest.GetResponseAsync()).ReturnsAsync((WebResponse)fakeResponse);
Mock.Arrange(() => mockWebRequest.GetResponse()).Returns((WebResponse)fakeResponse);
return mockWebRequest;
});

// Act
var response = await _client.SendAsync(_request);
var response = _client.Send(_request);

// Assert
Assert.That(response, Is.Not.Null);
Expand All @@ -83,7 +80,7 @@ public void SendAsync_ShouldThrow_WhenNullOutputStream()
});

// Act & Assert
Assert.ThrowsAsync<NullReferenceException>(() => _client.SendAsync(_request));
Assert.Throws<NullReferenceException>(() => _client.Send(_request));
}

[Test]
Expand All @@ -95,15 +92,15 @@ public void SendAsync_ThrowsWebException_WhenWebExceptionResponseIsNull()
var mockWebRequest = Mock.Create<HttpWebRequest>();
Mock.Arrange(() => mockWebRequest.Address).Returns(new Uri("https://sometesthost.com"));
var webException = new WebException("testing");
Mock.Arrange(() => mockWebRequest.GetResponseAsync()).Throws(webException);
Mock.Arrange(() => mockWebRequest.GetResponse()).Throws(webException);
return mockWebRequest;
});

// Act & Assert
Assert.ThrowsAsync<WebException>(() => _client.SendAsync(_request));
Assert.Throws<WebException>(() => _client.Send(_request));
}
[Test]
public async Task SendAsync_ReturnsResponse_WhenWebExceptionResponseIsNotNull()
public void Send_ReturnsResponse_WhenWebExceptionResponseIsNotNull()
{
// Arrange
_client.SetHttpWebRequestFunc(uri =>
Expand All @@ -115,12 +112,12 @@ public async Task SendAsync_ReturnsResponse_WhenWebExceptionResponseIsNotNull()
Mock.Arrange(() => mockHttpWebResponse.StatusCode).Returns(HttpStatusCode.BadRequest);
Mock.Arrange(() => mockHttpWebResponse.StatusDescription).Returns("Bad Request");
var webException = new WebException("testing", null, WebExceptionStatus.SendFailure,mockHttpWebResponse);
Mock.Arrange(() => mockWebRequest.GetResponseAsync()).Throws(webException);
Mock.Arrange(() => mockWebRequest.GetResponse()).Throws(webException);
return mockWebRequest;
});

// Act
var response = await _client.SendAsync(_request);
var response = _client.Send(_request);

// Assert
Assert.That(response, Is.Not.Null);
Expand Down
Loading
Loading