From f7144e5306756feb54bd0fb0b8ddf83046e4790b Mon Sep 17 00:00:00 2001 From: Russ Cam Date: Fri, 5 Jul 2019 18:51:41 +1000 Subject: [PATCH 1/2] Do not set HttpRequestMessage content when no PostData This commit changes the HttpConnection to not set Content on HttpRequestMessage when there is no PostData. The check is performed inside of Request and RequestAsync to avoid creating any content instance and allocations. Change length to default in TryComputeLength to match HttpContent implementation. Pass RequestData through to _onStreamAvailable delegates so that HttpCompression, ConnectionSettings and PostData can be accessed on the passed instance. Fixes #3907 --- .../Connection/Content/RequestDataContent.cs | 38 ++++++++++--------- .../Connection/HttpConnection.cs | 12 ++++-- src/Tests/Tests.Reproduce/GithubIssue3907.cs | 30 +++++++++++++++ 3 files changed, 60 insertions(+), 20 deletions(-) create mode 100644 src/Tests/Tests.Reproduce/GithubIssue3907.cs diff --git a/src/Elasticsearch.Net/Connection/Content/RequestDataContent.cs b/src/Elasticsearch.Net/Connection/Content/RequestDataContent.cs index 9ff4376a060..91baabd083a 100644 --- a/src/Elasticsearch.Net/Connection/Content/RequestDataContent.cs +++ b/src/Elasticsearch.Net/Connection/Content/RequestDataContent.cs @@ -25,8 +25,7 @@ namespace Elasticsearch.Net internal class RequestDataContent : HttpContent { private readonly RequestData _requestData; - private readonly Func _onStreamAvailable; - + private readonly Func _onStreamAvailable; public RequestDataContent(RequestData requestData) { @@ -35,14 +34,17 @@ public RequestDataContent(RequestData requestData) if (requestData.HttpCompression) Headers.ContentEncoding.Add("gzip"); - Task OnStreamAvailable(PostData data, Stream stream, HttpContent content, TransportContext context) + Task OnStreamAvailable(RequestData data, Stream stream, HttpContent content, TransportContext context) { - if (_requestData.HttpCompression) + if (data.HttpCompression) stream = new GZipStream(stream, CompressionMode.Compress, false); + using(stream) - data.Write(stream, requestData.ConnectionSettings); + data.PostData.Write(stream, data.ConnectionSettings); + return Task.CompletedTask; } + _onStreamAvailable = OnStreamAvailable; } public RequestDataContent(RequestData requestData, CancellationToken token) @@ -52,13 +54,15 @@ public RequestDataContent(RequestData requestData, CancellationToken token) if (requestData.HttpCompression) Headers.ContentEncoding.Add("gzip"); - async Task OnStreamAvailable(PostData data, Stream stream, HttpContent content, TransportContext context) + async Task OnStreamAvailable(RequestData data, Stream stream, HttpContent content, TransportContext context) { - if (_requestData.HttpCompression) + if (data.HttpCompression) stream = new GZipStream(stream, CompressionMode.Compress, false); + using (stream) - await data.WriteAsync(stream, requestData.ConnectionSettings, token).ConfigureAwait(false); + await data.PostData.WriteAsync(stream, data.ConnectionSettings, token).ConfigureAwait(false); } + _onStreamAvailable = OnStreamAvailable; } @@ -73,14 +77,9 @@ async Task OnStreamAvailable(PostData data, Stream stream, HttpContent content, [SuppressMessage("Microsoft.Design", "CA1031:DoNotCatchGeneralExceptionTypes", Justification = "Exception is passed as task result.")] protected override async Task SerializeToStreamAsync(Stream stream, TransportContext context) { - - var data = _requestData.PostData; - if (data == null) return; - var serializeToStreamTask = new TaskCompletionSource(); - var wrappedStream = new CompleteTaskOnCloseStream(stream, serializeToStreamTask); - await _onStreamAvailable(data, wrappedStream, this, context).ConfigureAwait(false); + await _onStreamAvailable(_requestData, wrappedStream, this, context).ConfigureAwait(false); await serializeToStreamTask.Task.ConfigureAwait(false); } @@ -92,7 +91,7 @@ protected override async Task SerializeToStreamAsync(Stream stream, TransportCon protected override bool TryComputeLength(out long length) { // We can't know the length of the content being pushed to the output stream. - length = -1; + length = default; return false; } @@ -113,8 +112,11 @@ protected override void Dispose(bool disposing) base.Dispose(); } - - public override void Close() => _serializeToStreamTask.TrySetResult(true); + public override void Close() + { + _serializeToStreamTask.TrySetResult(true); + base.Close(); + } } /// @@ -195,6 +197,8 @@ public override IAsyncResult BeginWrite(byte[] buffer, int offset, int count, As public override void EndWrite(IAsyncResult asyncResult) => _innerStream.EndWrite(asyncResult); public override void WriteByte(byte value) => _innerStream.WriteByte(value); + + public override void Close() => _innerStream.Close(); } } } diff --git a/src/Elasticsearch.Net/Connection/HttpConnection.cs b/src/Elasticsearch.Net/Connection/HttpConnection.cs index 92559112024..388418ab344 100644 --- a/src/Elasticsearch.Net/Connection/HttpConnection.cs +++ b/src/Elasticsearch.Net/Connection/HttpConnection.cs @@ -57,7 +57,10 @@ public virtual TResponse Request(RequestData requestData) try { var requestMessage = CreateHttpRequestMessage(requestData); - SetContent(requestMessage, requestData); + + if (requestData.PostData != null) + SetContent(requestMessage, requestData); + using(requestMessage?.Content ?? (IDisposable)Stream.Null) using (var d = DiagnosticSource.Diagnose(DiagnosticSources.HttpConnection.SendAndReceiveHeaders, requestData)) { @@ -107,8 +110,11 @@ public virtual async Task RequestAsync(RequestData request try { var requestMessage = CreateHttpRequestMessage(requestData); - SetAsyncContent(requestMessage, requestData, cancellationToken); - using(requestMessage?.Content ?? (IDisposable)Stream.Null) + + if (requestData.PostData != null) + SetAsyncContent(requestMessage, requestData, cancellationToken); + + using(requestMessage?.Content ?? (IDisposable)Stream.Null) using (var d = DiagnosticSource.Diagnose(DiagnosticSources.HttpConnection.SendAndReceiveHeaders, requestData)) { responseMessage = await client.SendAsync(requestMessage, HttpCompletionOption.ResponseHeadersRead, cancellationToken).ConfigureAwait(false); diff --git a/src/Tests/Tests.Reproduce/GithubIssue3907.cs b/src/Tests/Tests.Reproduce/GithubIssue3907.cs new file mode 100644 index 00000000000..0b0b822ae72 --- /dev/null +++ b/src/Tests/Tests.Reproduce/GithubIssue3907.cs @@ -0,0 +1,30 @@ +using System; +using System.Net; +using Elastic.Xunit.XunitPlumbing; +using FluentAssertions; +using Nest; +using Tests.Core.ManagedElasticsearch.Clusters; +using Tests.Domain; + +namespace Tests.Reproduce +{ + public class GithubIssue3907 : IClusterFixture + { + private readonly IntrusiveOperationCluster _cluster; + + // use intrusive operation because we're changing the underlying http handler + public GithubIssue3907(IntrusiveOperationCluster cluster) => _cluster = cluster; + + [I] + public void NotUsingSocketsHttpHandlerDoesNotCauseException() + { + AppContext.SetSwitch("System.Net.Http.UseSocketsHttpHandler", false); + + var response = _cluster.Client.Indices.Exists("non_existent_index"); + response.ApiCall.HttpStatusCode.Should().Be(404); + response.OriginalException.Should().BeNull(); + + AppContext.SetSwitch("System.Net.Http.UseSocketsHttpHandler", true); + } + } +} From 886b1b399b55f62aae66f1bdea234300b062ec12 Mon Sep 17 00:00:00 2001 From: Russ Cam Date: Mon, 8 Jul 2019 11:47:33 +1000 Subject: [PATCH 2/2] Address PR comments --- .../Connection/Content/RequestDataContent.cs | 8 ++------ src/Tests/Tests.Reproduce/GithubIssue3907.cs | 4 +++- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/Elasticsearch.Net/Connection/Content/RequestDataContent.cs b/src/Elasticsearch.Net/Connection/Content/RequestDataContent.cs index 91baabd083a..a85afcce514 100644 --- a/src/Elasticsearch.Net/Connection/Content/RequestDataContent.cs +++ b/src/Elasticsearch.Net/Connection/Content/RequestDataContent.cs @@ -91,7 +91,7 @@ protected override async Task SerializeToStreamAsync(Stream stream, TransportCon protected override bool TryComputeLength(out long length) { // We can't know the length of the content being pushed to the output stream. - length = default; + length = -1; return false; } @@ -112,11 +112,7 @@ protected override void Dispose(bool disposing) base.Dispose(); } - public override void Close() - { - _serializeToStreamTask.TrySetResult(true); - base.Close(); - } + public override void Close() => _serializeToStreamTask.TrySetResult(true); } /// diff --git a/src/Tests/Tests.Reproduce/GithubIssue3907.cs b/src/Tests/Tests.Reproduce/GithubIssue3907.cs index 0b0b822ae72..e958e5f7f34 100644 --- a/src/Tests/Tests.Reproduce/GithubIssue3907.cs +++ b/src/Tests/Tests.Reproduce/GithubIssue3907.cs @@ -12,7 +12,9 @@ public class GithubIssue3907 : IClusterFixture { private readonly IntrusiveOperationCluster _cluster; - // use intrusive operation because we're changing the underlying http handler + // use intrusive operation cluster because we're changing the underlying http handler + // and this cluster runs with a max concurrency of 1, so changing http handler + // will not affect other integration tests public GithubIssue3907(IntrusiveOperationCluster cluster) => _cluster = cluster; [I]