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.

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

[browser] [wasm] Remove manual ThrowIfCancellationRequested from ReadAsync response streaming #91834

Merged

Conversation

campersau
Copy link
Contributor

@campersau campersau commented Sep 9, 2023

While working on #91699 I noticed that there was one manual call to ThrowIfCancellationRequested in ReadAsync when doing response streaming, which means that this cancellation token does not go through the CancelationHelper which means that this won't abort the actual fetch request.

It normally shouldn't be an issue because developers are supposed to dispose the HttpResponseMessage / Stream which both would abort the fetch request.

With this PR ReadAsync would also abort the fetch request:

var request = new HttpRequestMessage(HttpMethod.Get, "https://someurl");
request.SetBrowserResponseStreamingEnabled(true);

var response = await new HttpClient().SendAsync(request, HttpCompletionOption.ResponseHeadersRead); // no using / dispose
var stream = await response.Content.ReadAsStreamAsync(); // no using / dispose
await stream.ReadAsync(Array.Emtpy<byte>(), new CancellationToken(true)); // now aborts fetch!

All other methods which could have called ThrowIfCancellationRequested also didn't and correctly go through CancelationHelper.

private async ValueTask<byte[]> GetResponseData(CancellationToken cancellationToken)

protected override Task SerializeToStreamAsync(Stream stream, TransportContext? context, CancellationToken cancellationToken)

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Sep 9, 2023
@ghost
Copy link

ghost commented Sep 9, 2023

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

While working on #91699 I noticed that there was one manual call to ThrowIfCancellationRequested in ReadAsync when doing response streaming, which means that this cancellation token does not go through the CancelationHelper which means that this won't abort the actual fetch request.

It normally shouldn't be an issue because developers are supposed to dispose the HttpResponseMessage / Stream which both would abort the fetch request.

With this PR this would also abort the fetch request:

var request = new HttpRequestMessage(HttpMethod.Get, "https://someurl");
request.SetBrowserResponseStreamingEnabled(true);

var response = await new HttpClient().SendAsync(request, HttpCompletionOption.ResponseHeadersRead); // no using / dispose
var stream = await response.Content.ReadAsStreamAsync(); // no using / dispose
await stream.ReadAsync(Array.Emtpy<byte>(), new CancellationToken(true)); // now aborts fetch!
Author: campersau
Assignees: -
Labels:

area-System.Net.Http, community-contribution

Milestone: -

@lewing lewing requested a review from pavelsavara September 11, 2023 02:00
@pavelsavara pavelsavara merged commit 38445c3 into dotnet:main Sep 11, 2023
@campersau campersau deleted the browserremoveThrowIfCancellationRequested branch September 11, 2023 12:55
@karelz karelz added this to the 9.0.0 milestone Sep 14, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Oct 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Http community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants