-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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] Make response streaming opt-out #111680
Conversation
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 support this in principle. I'm concerned about how the breaking change will surface to people upgrading. My assumption is that it at the moment it will only show up at runtime?
Yes, it will only show up at runtime. Similar to Kestrels Could we add an app context switch to toggle this globally? |
src/libraries/System.Net.Http/src/System/Net/Http/BrowserHttpHandler/BrowserHttpHandler.cs
Outdated
Show resolved
Hide resolved
Tagging subscribers to 'arch-wasm': @lewing |
We could make it ILLink feature. I'm not sure if making the breaking change is worth it. Most people don't stream large data.
|
FWIW this shouldn't be specific to just streaming large data. |
The GetFromJsonAsAsyncEnumerable case is very compelling to me given that the overhead of processing large json blobs is particularly painful in both memory and responsiveness in browser with a single thread. |
To me it looks like everyone supports this change. I guess it is still early to try it out in the previews then? |
@campersau I think we are missing the global opt-out linker feature. Are you willing to improve the PR ? |
I will work on adding global-opt out soon. @campersau I will take over this branch, ok ?
If the developer didn't opt out, I can see few options: A) at the start of request we can detect old browser and fail the request with some message hinting about possibilities how to opt out. Thoughts ? |
Okay, I have tried to figure out how to do the linker opt out, but it is hard to find all the places which need to be updated.
The runtime/src/libraries/System.Net.Http/src/System/Net/Http/BrowserHttpHandler/BrowserHttpHandler.cs Line 474 in bfc8814
I think all supported browsers support response streaming so I am not sure how you will detect it. The |
Yes, no worries.
Use the same detection we have there already. To detect unsupported browser and proceed or fail in meaningful way. |
FYI @guardrex |
@pavelsavara ... Is the new opt-out approach ... request.Options.Set(new HttpRequestOptionsKey<bool>("WebAssemblyEnableStreamingResponse"), false); ... landing at Preview 2? |
- System.Net.Http.WasmEnableStreamingResponse linker feature - DOTNET_WASM_ENABLE_STREAMING_RESPONSE env variable - tests & docs
- System.Net.Http.WasmEnableStreamingResponse linker feature - DOTNET_WASM_ENABLE_STREAMING_RESPONSE env variable - tests & docs
@guardrex probably Preview 3. I added some text in I also updated description on top of this PR |
Thanks @pavelsavara. I'll react to the updated description on the docs PR next week, and I'll ping for docs PR review later. |
@campersau do you have any comments, I think this is ready |
@@ -4,4 +4,9 @@ | |||
<method signature="System.Boolean IsGloballyEnabled()" body="stub" value="false" feature="System.Net.Http.EnableActivityPropagation" featurevalue="false" /> | |||
</type> | |||
</assembly> | |||
<assembly fullname="System.Net.Http" feature="System.Net.Http.WebAssemblyEnableStreamingResponse" featurevalue="false"> | |||
<type fullname="System.Net.Http.BrowserHttpController"> | |||
<method signature="System.Boolean get_FeatureEnableStreamingResponse()" body="stub" value="false" /> |
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.
What exactly is value="false"
/ featurevalue="false"
? The default value should be true
or is this something else?
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.
This means: if you set System.Net.Http.WebAssemblyEnableStreamingResponse
to false
then ILLink will stub the get_FeatureEnableStreamingResponse
to return false
instead of reading the config/env.
We don't want to also hardcode true
do we ?
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.
Is the behavior here the same in development as in publish?
We have something similar in Blazor routing and we had to do extra work for the development case
https://github.com/dotnet/aspnetcore/blob/main/src/Components/Components/src/Properties/ILLink.Substitutions.xml#L6-L8 for routing constraints, and it doesn't work in development unless you specifically set the AppContextSwitch manually
My understanding is that runtimeConfig.json
stuff doesn't flow to wasm at the moment.
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.
@@ -11,11 +11,26 @@ namespace System.Net.Http | |||
{ | |||
internal static partial class BrowserHttpInterop | |||
{ | |||
private static bool? _SupportsStreamingRequest; | |||
private static bool? _SupportsStreamingResponse; |
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.
Since this is now cached in .NET we don't need to cache it in JS anymore. Does not hurt though and might still be relevant for MT.
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 removed it there, thanks. I believe that the webworker has the same feature set as UI thread.
If not, having static cache in C# side would be wrong.
Co-authored-by: campersau <buchholz.bastian@googlemail.com>
This is now dependent on #97449 because without it the Blazor dev loop would not reflect the AppContext feature switch. |
@lewing I think we should merge it into P3 anyway. Quite likely dev-loop users do have latest browsers. |
/ba-g CI fails are unrelated |
This PR makes browser HTTP client to support streaming HTTP response by default. Until now, this feature was opt-in.
Because all evergreen browsers now support it.
This is compelling because it will make the
GetFromJsonAsAsyncEnumerable
streaming and lower memory consumption for large requests.breaking change
This is a breaking change because the
response.Content.ReadAsStreamAsync()
is no longerMemoryStream
butBrowserHttpReadStream
which doesn't support synchronous operations likeStream.Read(Span<Byte>)
. If your code uses synchronous operations, you can disable the feature or copy the stream intoMemoryStream
yourself.how to opt-out
If you need to disable it globally, you can use
<WasmEnableStreamingResponse>false</WasmEnableStreamingResponse>
in your project fileDOTNET_WASM_ENABLE_STREAMING_RESPONSE
To disable it for individual request, you can use existing
request.Options.Set(new HttpRequestOptionsKey<bool>("WebAssemblyEnableStreamingResponse"), false);
request.SetBrowserResponseStreamingEnabled(false)
extension in Blazor projectRelated to #77904
Fixes to #112442