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.

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

Jump to bottom

Add CancellationToken support for LoadIntoBufferAsync #103991

Merged
merged 5 commits into from
Jul 8, 2024
Merged

Add CancellationToken support for LoadIntoBufferAsync #103991

Show file tree
Hide file tree
Changes from 1 commit
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
4 changes: 2 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,10 @@ public Task LoadIntoBufferAsync() =>
public Task LoadIntoBufferAsync(long maxBufferSize) =>
LoadIntoBufferAsync(maxBufferSize, CancellationToken.None);

internal Task LoadIntoBufferAsync(CancellationToken cancellationToken) =>
public Task LoadIntoBufferAsync(CancellationToken cancellationToken) =>
stephentoub marked this conversation as resolved.
Show resolved Hide resolved
LoadIntoBufferAsync(MaxBufferSize, cancellationToken);

internal Task LoadIntoBufferAsync(long maxBufferSize, CancellationToken cancellationToken)
public Task LoadIntoBufferAsync(long maxBufferSize, CancellationToken cancellationToken)
{
CheckDisposed();

Expand Down
92 changes: 92 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,98 @@ 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);

var cts = new CancellationTokenSource();
cts.Cancel();

await response.Content.LoadIntoBufferAsync(cts.Token);
},
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);

var cts = new CancellationTokenSource();
cts.Cancel();
Copy link
Member

Choose a reason for hiding this comment

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

If you're not going to check the identity of the token, this can just be new CancellationToken(true). But if we expect the resulting exception to contain this token, then we should also be validating that after the below assert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests updated accordingly.


await Assert.ThrowsAsync<TaskCanceledException>(() => response.Content.LoadIntoBufferAsync(cts.Token));
},
async server =>
{
try
{
await server.AcceptConnectionSendResponseAndCloseAsync();
}
catch (Exception ex)
{
_output.WriteLine($"Ignored exception:{Environment.NewLine}{ex}");
}
MihaZupan marked this conversation as resolved.
Show resolved Hide resolved
});
}

[Fact]
public async Task LoadIntoBufferAsync_Unbuffered_CanBeCanceled()
{
var cts = new CancellationTokenSource();

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

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

await Assert.ThrowsAsync<TaskCanceledException>(() => response.Content.LoadIntoBufferAsync(cts.Token));
},
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

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

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

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.

try
{
await connection.SendResponseAsync(new string('a', 100));
}
catch (Exception ex)
{
_output.WriteLine($"Ignored exception:{Environment.NewLine}{ex}");
}
MihaZupan marked this conversation as resolved.
Show resolved Hide resolved
});
});
}

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