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

[Wasm] WebAssemblyHttpRequestMessageExtensions should be part of the BCL #77904

Open
jeromelaban opened this issue Nov 4, 2022 · 11 comments
Open
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation arch-wasm WebAssembly architecture area-System.Net.Http
Milestone

Comments

@jeromelaban
Copy link
Contributor

Blazor provides as part of Microsoft.AspNetCore.Components.WebAssembly a set of extensions (docs) that augment the features of HttpRequestMessage like SetBrowserRequestCredentials or SetBrowserRequestOption specifically for WebAssembly.

Those extensions do not have anything special to Blazor (even if the package depends on Blazor), and would benefit for being available in BCL, so that non-blazor frameworks (Such as Uno Platform) can benefit from those extensions without having to import those 4 types locally.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Nov 4, 2022
@radical radical added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Http labels Nov 4, 2022
@ghost
Copy link

ghost commented Nov 4, 2022

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

Issue Details

Blazor provides as part of Microsoft.AspNetCore.Components.WebAssembly a set of extensions (docs) that augment the features of HttpRequestMessage like SetBrowserRequestCredentials or SetBrowserRequestOption specifically for WebAssembly.

Those extensions do not have anything special to Blazor (even if the package depends on Blazor), and would benefit for being available in BCL, so that non-blazor frameworks (Such as Uno Platform) can benefit from those extensions without having to import those 4 types locally.

Author: jeromelaban
Assignees: -
Labels:

api-suggestion, area-System.Net.Http, untriaged

Milestone: -

@radical
Copy link
Member

radical commented Nov 4, 2022

cc @lewing @pavelsavara @javiercn

@rzikm rzikm added the arch-wasm WebAssembly architecture label Nov 7, 2022
@ghost
Copy link

ghost commented Nov 7, 2022

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

Blazor provides as part of Microsoft.AspNetCore.Components.WebAssembly a set of extensions (docs) that augment the features of HttpRequestMessage like SetBrowserRequestCredentials or SetBrowserRequestOption specifically for WebAssembly.

Those extensions do not have anything special to Blazor (even if the package depends on Blazor), and would benefit for being available in BCL, so that non-blazor frameworks (Such as Uno Platform) can benefit from those extensions without having to import those 4 types locally.

Author: jeromelaban
Assignees: -
Labels:

api-suggestion, arch-wasm, area-System.Net.Http, untriaged

Milestone: -

@marek-safar marek-safar added this to the 8.0.0 milestone Nov 7, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Nov 7, 2022
@lewing lewing modified the milestones: 8.0.0, 9.0.0 Jul 25, 2023
@pavelsavara
Copy link
Member

Should we make request/response streaming opt-out instead of opt-in ?
What would be the consequences for existing code ?
What would be the consequences for code tested on chrome and run elsewhere ?

cc @campersau @maraf

@maraf
Copy link
Member

maraf commented Sep 1, 2023

I would be good to start with perf measurements

@pavelsavara
Copy link
Member

Do we have any perf measurements?

We have some WebSocket perf benchmark about copy buffer efficiency.
WS is in range of 0.03 ms per small chunk.
Such perf may be relevant if user write many small chunks into the stream.

But then this involves network latencies which are orders of magnitude bigger.

@campersau
Copy link
Contributor

campersau commented Sep 1, 2023

Should we make request/response streaming opt-out instead of opt-in ?

For response streaming opt-out seems ok to me.
For request streaming I would keep it opt-in because we can't detect if the server supports HTTP/2 or higher.

What would be the consequences for existing code ?

Since there is HttpCompletionOption.ResponseContentRead and it is the default, .NET is buffering anyway in those cases. And HttpCompletionOption.ResponseHeadersRead would do what the user actually wants by default. So 👍
Maybe we should enable/disable response streaming based on this setting instead by default?

What would be the consequences for code tested on chrome and run elsewhere ?

Since response streaming seems to be supported everywhere it should be fine.

@campersau
Copy link
Contributor

I did some performance tests with a blazor webassembly app on .NET 9.0.100-rc.2.24474.11.
At around 1000000 bytes the performance degrades for response streaming using a buffer half the size of the requested data when reading.

Bytes ResponseStreamingEnabled(false) + ResponseHeadersRead ResponseStreamingEnabled(true) + ResponseHeadersRead ResponseStreamingEnabled(false) + ResponseContentRead ResponseStreamingEnabled(true) + ResponseContentRead
0 2.7ms 2.7ms 2.8ms 2.8ms
1 2.9ms 3.1ms 2.9ms 3.1ms
10 2.9ms 3.1ms 2.9ms 3.1ms
100 2.9ms 3.1ms 2.9ms 3.2ms
1000 2.9ms 3.1ms 3.1ms 3.2ms
10000 2.9ms 3.2ms 3.7ms 3.9ms
100000 3.8ms 4.1ms 4.8ms 5.0ms
1000000 9.0ms 14.8ms 10.6ms 24.5ms
10000000 62.8ms 426.7ms 65.0ms 283.4ms
100000000 490.2ms 2432.2ms 520.0ms 1840.7ms
1000000000 OOM 6139.3ms OOM OOM
var buffer = new byte[Math.Max(1, size / 2)];
using var request = new HttpRequestMessage(HttpMethod.Get, $"http://localhost:5219/{size}");
request.SetBrowserResponseStreamingEnabled(streaming);
using var response = await new HttpClient().SendAsync(request, httpCompletionOption);
using var stream = await response.Content.ReadAsStreamAsync();
while (await stream.ReadAsync(buffer) != 0) {}

@MihaZupan
Copy link
Member

As another +1 for considering changing the default here, #110287 is another case bit by the difference in behavior.
Buffering by default makes accelerator APIs like GetFromJsonAsAsyncEnumerable useless on browser where the user must instead write something like this:

var request = new HttpRequestMessage(HttpMethod.Get, url);
request.SetBrowserResponseStreamingEnabled(true);
using var response = await HttpClient.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, ct);
var itemStream = response.Content.ReadFromJsonAsAsyncEnumerable<string>(ct);

@NetherGranite
Copy link

NetherGranite commented Dec 1, 2024

Buffering by default makes accelerator APIs like GetFromJsonAsAsyncEnumerable useless on browser where the user must instead write something like this...

If changing the default isn't preferrable, I do think at the very least it would make much more sense than not to make it the default behavior for GetFromJsonAsAsyncEnumerable specifically. It not being the default for that method feels like a pit of failure. Is there even a strong use case for that method that isn't streaming?

Edit: I assume that the reason for this is SetBrowserResponseStreamingEnabled is part of a different stack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation arch-wasm WebAssembly architecture area-System.Net.Http
Projects
None yet
Development

No branches or pull requests

10 participants