Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Commit

Permalink
Merge pull request #19082 from stephentoub/stop_disposing_content
Browse files Browse the repository at this point in the history 
Stop disposing of request content in HttpClient
  • Loading branch information
@stephentoub
stephentoub authored Apr 28, 2017
2 parents c311292 + 6d06c39 commit e9318bc
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 148 deletions.
39 changes: 24 additions & 15 deletions src/System.Net.Http/src/System/Net/Http/HttpClient.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -463,7 +463,7 @@ private async Task<HttpResponseMessage> FinishSendAsyncBuffered(
}
finally
{
HandleFinishSendAsyncCleanup(request, cts, disposeCts);
HandleFinishSendAsyncCleanup(cts, disposeCts);
}
}

Expand All @@ -488,7 +488,7 @@ private async Task<HttpResponseMessage> FinishSendAsyncUnbuffered(
}
finally
{
HandleFinishSendAsyncCleanup(request, cts, disposeCts);
HandleFinishSendAsyncCleanup(cts, disposeCts);
}
}

Expand All @@ -505,22 +505,31 @@ private void HandleFinishSendAsyncError(Exception e, CancellationTokenSource cts
}
}

private void HandleFinishSendAsyncCleanup(HttpRequestMessage request, CancellationTokenSource cts, bool disposeCts)
private void HandleFinishSendAsyncCleanup(CancellationTokenSource cts, bool disposeCts)
{
try
// Dispose of the CancellationTokenSource if it was created specially for this request
// rather than being used across multiple requests.
if (disposeCts)
{
// When a request completes, dispose the request content so the user doesn't have to. This also
// helps ensure that a HttpContent object is only sent once using HttpClient (similar to HttpRequestMessages
// that can also be sent only once).
request.Content?.Dispose();
}
finally
{
if (disposeCts)
{
cts.Dispose();
}
cts.Dispose();
}

// This method used to also dispose of the request content, e.g.:
// request.Content?.Dispose();
// This has multiple problems:
// 1. It prevents code from reusing request content objects for subsequent requests,
// as disposing of the object likely invalidates it for further use.
// 2. It prevents the possibility of partial or full duplex communication, even if supported
// by the handler, as the request content may still be in use even if the response
// (or response headers) has been received.
// By changing this to not dispose of the request content, disposal may end up being
// left for the finalizer to handle, or the developer can explicitly dispose of the
// content when they're done with it. But it allows request content to be reused,
// and more importantly it enables handlers that allow receiving of the response before
// fully sending the request. Prior to this change, a handler like CurlHandler would
// fail trying to access certain sites, if the site sent its response before it had
// completely received the request: CurlHandler might then find that the request content
// was disposed of while it still needed to read from it.
}

public void CancelPendingRequests()
Expand Down
140 changes: 9 additions & 131 deletions src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1380,147 +1380,25 @@ public async Task PostAsync_Redirect_LargePayload(int statusCode, bool expectRed
}
}

[OuterLoop] // TODO: Issue #11345
[Fact]
public async Task PostAsync_ResponseContentRead_RequestContentDisposedAfterResponseBuffered()
{
using (var client = new HttpClient())
{
await LoopbackServer.CreateServerAsync(async (server, url) =>
{
bool contentDisposed = false;
Task<HttpResponseMessage> post = client.SendAsync(new HttpRequestMessage(HttpMethod.Post, url)
{
Content = new DelegateContent
{
SerializeToStreamAsyncDelegate = (contentStream, contentTransport) => contentStream.WriteAsync(new byte[100], 0, 100),
TryComputeLengthDelegate = () => Tuple.Create<bool, long>(true, 100),
DisposeDelegate = _ => contentDisposed = true
}
}, HttpCompletionOption.ResponseContentRead);
await LoopbackServer.AcceptSocketAsync(server, async (s, stream, reader, writer) =>
{
// Read headers from client
while (!string.IsNullOrEmpty(await reader.ReadLineAsync())) ;
// Send back all headers and some but not all of the response
await writer.WriteAsync($"HTTP/1.1 200 OK\r\nDate: {DateTimeOffset.UtcNow:R}\r\nContent-Length: 10\r\n\r\n");
await writer.WriteAsync("abcd"); // less than contentLength
// The request content should not be disposed of until all of the response has been sent
await Task.Delay(1); // a little time to let data propagate
Assert.False(contentDisposed, "Expected request content not to be disposed");
// Send remaining response content
await writer.WriteAsync("efghij");
s.Shutdown(SocketShutdown.Send);

// The task should complete and the request content should be disposed
using (HttpResponseMessage response = await post)
{
Assert.True(contentDisposed, "Expected request content to be disposed");
Assert.Equal("abcdefghij", await response.Content.ReadAsStringAsync());
}
return null;
});
});
}
}

[OuterLoop] // TODO: Issue #11345
[Fact]
public async Task PostAsync_ResponseHeadersRead_RequestContentDisposedAfterRequestFullySentAndResponseHeadersReceived()
[Theory, MemberData(nameof(EchoServers))] // NOTE: will not work for in-box System.Net.Http.dll due to disposal of request content
public async Task PostAsync_ReuseRequestContent_Success(Uri remoteServer)
{
const string ContentString = "This is the content string.";
using (var client = new HttpClient())
{
await LoopbackServer.CreateServerAsync(async (server, url) =>
var content = new StringContent(ContentString);
for (int i = 0; i < 2; i++)
{
var trigger = new TaskCompletionSource<bool>(TaskCreationOptions.RunContinuationsAsynchronously);
bool contentDisposed = false;
Task<HttpResponseMessage> post = client.SendAsync(new HttpRequestMessage(HttpMethod.Post, url)
using (HttpResponseMessage response = await client.PostAsync(remoteServer, content))
{
Content = new DelegateContent
{
SerializeToStreamAsyncDelegate = async (contentStream, contentTransport) =>
{
await contentStream.WriteAsync(new byte[50], 0, 50);
await trigger.Task;
await contentStream.WriteAsync(new byte[50], 0, 50);
},
TryComputeLengthDelegate = () => Tuple.Create<bool, long>(true, 100),
DisposeDelegate = _ => contentDisposed = true
}
}, HttpCompletionOption.ResponseHeadersRead);
await LoopbackServer.AcceptSocketAsync(server, async (s, stream, reader, writer) =>
{
// Read headers from client
while (!string.IsNullOrEmpty(await reader.ReadLineAsync())) ;
// Send back all headers and some but not all of the response
await writer.WriteAsync($"HTTP/1.1 200 OK\r\nDate: {DateTimeOffset.UtcNow:R}\r\nContent-Length: 10\r\n\r\n");
await writer.WriteAsync("abcd"); // less than contentLength
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
Assert.False(contentDisposed, "Expected request content to not be disposed while request data still being sent");
}
else // [ActiveIssue(9006, TestPlatforms.AnyUnix)]
{
await post;
Assert.True(contentDisposed, "Current implementation will dispose of the request content once response headers arrive");
}
// Allow request content to complete
trigger.SetResult(true);
// Send remaining response content
await writer.WriteAsync("efghij");
s.Shutdown(SocketShutdown.Send);
// The task should complete and the request content should be disposed
using (HttpResponseMessage response = await post)
{
Assert.True(contentDisposed, "Expected request content to be disposed");
Assert.Equal("abcdefghij", await response.Content.ReadAsStringAsync());
}
return null;
});
});
}
}

private sealed class DelegateContent : HttpContent
{
internal Func<Stream, TransportContext, Task> SerializeToStreamAsyncDelegate;
internal Func<Tuple<bool, long>> TryComputeLengthDelegate;
internal Action<bool> DisposeDelegate;

protected override Task SerializeToStreamAsync(Stream stream, TransportContext context)
{
return SerializeToStreamAsyncDelegate != null ?
SerializeToStreamAsyncDelegate(stream, context) :
Task.CompletedTask;
}

protected override bool TryComputeLength(out long length)
{
if (TryComputeLengthDelegate != null)
{
var result = TryComputeLengthDelegate();
length = result.Item2;
return result.Item1;
Assert.Equal(HttpStatusCode.OK, response.StatusCode);
Assert.Contains(ContentString, await response.Content.ReadAsStringAsync());
}
}

length = 0;
return false;
}

protected override void Dispose(bool disposing) =>
DisposeDelegate?.Invoke(disposing);
}

[OuterLoop] // TODO: Issue #11345
Expand Down
4 changes: 2 additions & 2 deletions src/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -323,14 +323,14 @@ public async Task SendAsync_DuplicateRequest_ThrowsException()
}

[Fact]
public async Task SendAsync_RequestContentDisposed()
public async Task SendAsync_RequestContentNotDisposed()
{
var content = new ByteArrayContent(new byte[1]);
using (var request = new HttpRequestMessage(HttpMethod.Get, CreateFakeUri()) { Content = content })
using (var client = new HttpClient(new CustomResponseHandler((r, c) => Task.FromResult(new HttpResponseMessage()))))
{
await client.SendAsync(request);
Assert.Throws<ObjectDisposedException>(() => { content.ReadAsStringAsync(); });
await content.ReadAsStringAsync(); // no exception
}
}

Expand Down

0 comments on commit e9318bc

Please sign in to comment.