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

Add SetBrowserRequestStreamingEnabled extension method #50481

Merged

Conversation

campersau
Copy link
Contributor

Add SetBrowserRequestStreamingEnabled extension method

Browser request streaming was implemented in dotnet/runtime#91295. This adds the corresponding extension method for it.

Description

Browser request streaming requires HTTP/2 or higher server support and is currently only supported in Chromium (Chrome / Edge / ...) e.g. it does not work in Firefox or Safari.
See also https://developer.chrome.com/articles/fetch-streaming-requests/

CC @pavelsavara

@campersau campersau requested a review from a team as a code owner September 1, 2023 19:59
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-blazor Includes: Blazor, Razor Components label Sep 1, 2023
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Sep 1, 2023
@ghost
Copy link

ghost commented Sep 1, 2023

Thanks for your PR, @campersau. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@mkArtakMSFT
Copy link
Member

@MackinnonBuck can you please review this? Thanks!

@javiercn
Copy link
Member

javiercn commented Sep 5, 2023

What does it happen on unsupported browsers. Does it fallback transparently?

I'm a bit worried of adding this as it can create a performance cliff when not running on chrome/edge, which might be a nasty surprise when you run the app in Safari if you only tested on Chrome/Edge

@campersau
Copy link
Contributor Author

What does it happen on unsupported browsers. Does it fallback transparently?

Yes, it does fallback transparently. This is the same behavior as for response streaming.
With the difference being that response streaming is available in all browser now (which means we might enable it by default dotnet/runtime#77904 (comment)).

I would be fine with closing / delaying this PR until it is avaiable in more browsers.

@pavelsavara
Copy link
Member

I'm a bit worried of adding this as it can create a performance cliff

If this stays opt-in, then it's "use if if you know what you are doing", no ?

Maybe we could xml-doc describing the fallback behavior and perf/memory/blocking consequences.
Something like
Note: On browsers which don't support request streaming, the full request body would need to be produced first, before the actual request is made. This is same behavior as without this flag. It will also consume memory in size of full request. Please make sure that your applications is also working on browsers other than Chromium.

@matthewreiter
Copy link

I'm a bit worried of adding this as it can create a performance cliff

If this stays opt-in, then it's "use if if you know what you are doing", no ?

Maybe we could xml-doc describing the fallback behavior and perf/memory/blocking consequences. Something like Note: On browsers which don't support request streaming, the full request body would need to be produced first, before the actual request is made. This is same behavior as without this flag. It will also consume memory in size of full request. Please make sure that your applications is also working on browsers other than Chromium.

Related to this, it would be good to publicly expose whether streaming is supported (i.e. the result of calling BrowserHttpInterop.SupportsStreamingRequest) so that user code can add value in such a situation, such as showing an upload progress indicator based on how many bytes were read from the stream, which would only work properly when using streaming requests.

@javiercn
Copy link
Member

javiercn commented Sep 6, 2023

Since this is .NET 9.0 I have no objection on this going in right now, since we have time to adjust

@pavelsavara pavelsavara added this to the 9.0-preview1 milestone Sep 6, 2023
@ghost
Copy link

ghost commented Sep 14, 2023

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Sep 14, 2023
@danmoseley
Copy link
Member

@MackinnonBuck is this ready to merge or waiting on something else

@MackinnonBuck
Copy link
Member

@danmoseley Nope, this can be merged. I'm going to re-run CI first, though.

@MackinnonBuck
Copy link
Member

/azp run

@ghost ghost removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Sep 29, 2023
@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@ghost
Copy link

ghost commented Oct 7, 2023

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Oct 7, 2023
@MackinnonBuck
Copy link
Member

/azp run

@ghost ghost removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Oct 17, 2023
@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@MackinnonBuck MackinnonBuck enabled auto-merge (squash) October 17, 2023 16:13
@MackinnonBuck MackinnonBuck merged commit 662795d into dotnet:main Oct 17, 2023
26 checks passed
@campersau campersau deleted the SetBrowserRequestStreamingEnabled branch October 17, 2023 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components 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.

7 participants