-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Do not set HttpRequestMessage content when no PostData #3925
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,8 +25,7 @@ namespace Elasticsearch.Net | |
| internal class RequestDataContent : HttpContent | ||
| { | ||
| private readonly RequestData _requestData; | ||
| private readonly Func<PostData, CompleteTaskOnCloseStream, RequestDataContent, TransportContext, Task> _onStreamAvailable; | ||
|
|
||
| private readonly Func<RequestData, CompleteTaskOnCloseStream, RequestDataContent, TransportContext, Task> _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<bool>(); | ||
|
|
||
| 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(); | ||
|
||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
@@ -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(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does not implement this, I think its good to have though. |
||
| } | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<ReadOnlyCluster> | ||
| { | ||
| 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); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add comment this is safe to do because |
||
|
|
||
| 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); | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-1was taken from: https://github.com/aspnet/AspNetWebStack/blob/master/src/System.Net.Http.Formatting/PushStreamContent.cs#L128which
RequestDataContentis modeled after