-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add CancellationToken support for LoadIntoBufferAsync #103991
Conversation
Note regarding the
|
Note regarding the
|
Tagging subscribers to this area: @dotnet/ncl |
HttpCompletionOption.ResponseHeadersRead); | ||
|
||
var cts = new CancellationTokenSource(); | ||
cts.Cancel(); |
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.
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.
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.
Tests updated accordingly.
await connection.SendResponseAsync(LoopbackServer.GetHttpResponseHeaders(contentLength: 100)); | ||
await Task.Delay(250); | ||
cts.Cancel(); | ||
await Task.Delay(500); |
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.
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.
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.
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.
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.
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.
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.
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.
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.
Product changes LGTM. Thanks!
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.
Thanks
src/libraries/System.Net.Http/tests/FunctionalTests/HttpContentTest.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/tests/FunctionalTests/HttpContentTest.cs
Outdated
Show resolved
Hide resolved
await connection.SendResponseAsync(LoopbackServer.GetHttpResponseHeaders(contentLength: 100)); | ||
await Task.Delay(250); | ||
cts.Cancel(); | ||
await Task.Delay(500); |
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.
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.
Closes #102659