Skip to content

Add CancellationToken support for LoadIntoBufferAsync #103991

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jul 8, 2024
Merged

Add CancellationToken support for LoadIntoBufferAsync #103991

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cd3afd3
Add CancellationToken support for LoadIntoBufferAsync
manandre Jun 25, 2024
09ea276
Add documentation
manandre Jun 26, 2024
70ec5c1
Rework cancellation token in tests
manandre Jun 27, 2024
f08c166
Move to outerloop test using delays
manandre Jun 27, 2024
9d36763
Use IgnoreExceptions helper in tests
MihaZupan Jul 8, 2024
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
2 changes: 2 additions & 0 deletions src/libraries/System.Net.Http/ref/System.Net.Http.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,9 @@ public void CopyTo(System.IO.Stream stream, System.Net.TransportContext? context
public void Dispose() { }
protected virtual void Dispose(bool disposing) { }
public System.Threading.Tasks.Task LoadIntoBufferAsync() { throw null; }
public System.Threading.Tasks.Task LoadIntoBufferAsync(System.Threading.CancellationToken cancellationToken) { throw null; }
public System.Threading.Tasks.Task LoadIntoBufferAsync(long maxBufferSize) { throw null; }
public System.Threading.Tasks.Task LoadIntoBufferAsync(long maxBufferSize, System.Threading.CancellationToken cancellationToken) { throw null; }
public System.Threading.Tasks.Task<byte[]> ReadAsByteArrayAsync() { throw null; }
public System.Threading.Tasks.Task<byte[]> ReadAsByteArrayAsync(System.Threading.CancellationToken cancellationToken) { throw null; }
public System.IO.Stream ReadAsStream() { throw null; }
Expand Down
27 changes: 25 additions & 2 deletions src/libraries/System.Net.Http/src/System/Net/Http/HttpContent.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,10 +476,33 @@ public Task LoadIntoBufferAsync() =>
public Task LoadIntoBufferAsync(long maxBufferSize) =>
LoadIntoBufferAsync(maxBufferSize, CancellationToken.None);

internal Task LoadIntoBufferAsync(CancellationToken cancellationToken) =>
/// <summary>
/// Serialize the HTTP content to a memory buffer as an asynchronous operation.
/// </summary>
/// <param name="cancellationToken">The cancellation token to cancel the operation.</param>
/// <returns>The task object representing the asynchronous operation.</returns>
/// <remarks>
/// This operation will not block. The returned <see cref="Task"/> object will complete after all of the content has been serialized to the memory buffer.
/// After content is serialized to a memory buffer, calls to one of the <see cref="CopyToAsync(Stream)"/> methods will copy the content of the memory buffer to the target stream.
/// </remarks>
/// <exception cref="OperationCanceledException">The cancellation token was canceled. This exception is stored into the returned task.</exception>
/// <exception cref="ObjectDisposedException">The object has already been disposed.</exception>
public Task LoadIntoBufferAsync(CancellationToken cancellationToken) =>
LoadIntoBufferAsync(MaxBufferSize, cancellationToken);

internal Task LoadIntoBufferAsync(long maxBufferSize, CancellationToken cancellationToken)
/// <summary>
/// Serialize the HTTP content to a memory buffer as an asynchronous operation.
/// </summary>
/// <param name="maxBufferSize">The maximum size, in bytes, of the buffer to use.</param>
/// <param name="cancellationToken">The cancellation token to cancel the operation.</param>
/// <returns>The task object representing the asynchronous operation.</returns>
/// <remarks>
/// This operation will not block. The returned <see cref="Task"/> object will complete after all of the content has been serialized to the memory buffer.
/// After content is serialized to a memory buffer, calls to one of the <see cref="CopyToAsync(Stream)"/> methods will copy the content of the memory buffer to the target stream.
/// </remarks>
/// <exception cref="OperationCanceledException">The cancellation token was canceled. This exception is stored into the returned task.</exception>
/// <exception cref="ObjectDisposedException">The object has already been disposed.</exception>
public Task LoadIntoBufferAsync(long maxBufferSize, CancellationToken cancellationToken)
{
CheckDisposed();

Expand Down
87 changes: 87 additions & 0 deletions src/libraries/System.Net.Http/tests/FunctionalTests/HttpContentTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,93 @@ public async Task LoadIntoBufferAsync_ThrowIOExceptionInOverriddenAsyncMethod_Th
Assert.IsType<IOException>(ex.InnerException);
}

[Fact]
public async Task LoadIntoBufferAsync_Buffered_IgnoresCancellationToken()
{
string content = Guid.NewGuid().ToString();

await LoopbackServer.CreateClientAndServerAsync(
async uri =>
{
using HttpClient httpClient = CreateHttpClient();

HttpResponseMessage response = await httpClient.GetAsync(
uri,
HttpCompletionOption.ResponseContentRead);

CancellationToken cancellationToken = new CancellationToken(canceled: true);

await response.Content.LoadIntoBufferAsync(cancellationToken);
},
async server =>
{
await server.AcceptConnectionSendResponseAndCloseAsync(content: content);
});
}

[Fact]
public async Task LoadIntoBufferAsync_Unbuffered_CanBeCanceled_AlreadyCanceledCts()
{
await LoopbackServer.CreateClientAndServerAsync(
async uri =>
{
using HttpClient httpClient = CreateHttpClient();

HttpResponseMessage response = await httpClient.GetAsync(
uri,
HttpCompletionOption.ResponseHeadersRead);

CancellationToken cancellationToken = new CancellationToken(canceled: true);

Task task = response.Content.LoadIntoBufferAsync(cancellationToken);

var exception = await Assert.ThrowsAsync<TaskCanceledException>(() => task);

Assert.Equal(cancellationToken, exception.CancellationToken);
},
async server =>
{
await IgnoreExceptions(server.AcceptConnectionSendResponseAndCloseAsync());
});
}

[OuterLoop("Uses Task.Delay")]
[Fact]
public async Task LoadIntoBufferAsync_Unbuffered_CanBeCanceled()
{
var cts = new CancellationTokenSource();

await LoopbackServer.CreateClientAndServerAsync(
async uri =>
{
using HttpClient httpClient = base.CreateHttpClient();

HttpResponseMessage response = await httpClient.GetAsync(
uri,
HttpCompletionOption.ResponseHeadersRead);

CancellationToken cancellationToken = cts.Token;

Task task = response.Content.LoadIntoBufferAsync(cancellationToken);

var exception = await Assert.ThrowsAsync<TaskCanceledException>(() => task);

Assert.Equal(cancellationToken, exception.CancellationToken);
},
async server =>
{
await server.AcceptConnectionAsync(async connection =>
{
await connection.ReadRequestHeaderAsync();
await connection.SendResponseAsync(LoopbackServer.GetHttpResponseHeaders(contentLength: 100));
await Task.Delay(250);
cts.Cancel();
await Task.Delay(500);
Copy link
Member

@stephentoub stephentoub Jun 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do the delays need to be this long? Might want to mark the test as outerloop if there's no way to shrink them. Even better would be to find a way to make it more deterministic without timeouts / delays.

Copy link
Contributor Author

@manandre manandre Jun 27, 2024

Choose a reason for hiding this comment

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

I am not sure but I assume that these delays try to enforce the LoadIntoBuffer task to actively wait on the cancellation token.
For now I have moved it to outerloop, but ideas to make it deterministic are welcome.

Copy link
Member

@MihaZupan MihaZupan Jul 8, 2024

Choose a reason for hiding this comment

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

A deterministic version would look something like a revert of ef6a5a2.

var observedCancellation = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
var exception = await Assert.ThrowsAsync<TaskCanceledException>(() => task);
observedCancellation.SetResult();
await connection.SendResponseAsync(LoopbackServer.GetHttpResponseHeaders(contentLength: 100));
await Task.Yield();
cts.Cancel();
await observedCancellation.Task.WaitAsync(TestHelper.PassingTestTimeout);

But as written, this test isn't enforcing that the client actually honors the cancellation token before receiving the whole response.
The browser implementation seems to be struggling with this (I had to revert a similar change in #104384 due to test failures - console logs). cc: @pavelsavara

Given that this test is a copy-paste of existing test logic we have in this file, I'd keep it as-is in this PR.

Copy link
Member

@pavelsavara pavelsavara Jul 8, 2024

Choose a reason for hiding this comment

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

In the browser the server side is played by xharness and driven via WebSocket, that makes it lot slower to react to anything.

It could be probably rewritten in deterministic way, but it could be separate PR if desired.

await IgnoreExceptions(connection.SendResponseAsync(new string('a', 100)));
});
});
}

[Theory]
[InlineData(true)]
[InlineData(false)]
Expand Down