[browser] [wasm] Request Streaming upload #91295

campersau · 2023-08-29T22:38:02Z

Uses a ReadableStream as fetch request body and reads chunks from .NET HttpContent stream.
The feature needs to be enabled via HttpRequestOptionsKey<bool>("WebAssemblyEnableStreamingRequest"). A helper could be added to WebAssemblyHttpRequestMessageExtensions.
It is only supported using HTTP/2 or higher and is currently only implemented in Chromium (Chrome / Edge) (see #36634 (comment)).

It currently tries to get the buffer size from the browser via desiredSize which in my testing was always 0. We can also remove this detection and just use the fixed default buffer size.
I also enabled autoAllocateChunkSize which provides us a byobRequest so we can copy directly into the browsers request buffer. Unfortunately WebAssembly/design#1162 is not yet implemented which means we can't do a zero-byte transfer by passing the ArrayBuffer to .NET and avoid any buffers in .NET.
The default buffer size is currently set to 65536. Maybe it should also be made configurable via a HttpRequestOptionsKey?

Exceptions from reading the HttpContent stream will fail the request with the thrown exception.
The special case for StringContent is still preferred and bypasses streaming.

I added some BrowserHttpHandler_* tests to ResponseStreamTest.cs analog to the WebAssemblyEnableStreamingResponse one. Maybe these BrowserHttpHandler_* test should be moved in their own file?
I duplicated the PostAsync_ThrowFromContentCopy_RequsetFails tests to use remote server, as the Http2LoopbackServer does not work in the browser.

Do I need to adjust something regarding FEATURE_WASM_THREADS?

Fixes #36634

ghost · 2023-08-29T22:38:19Z

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

Issue Details

Uses a ReadableStream as fetch request body and reads chunks from .NET HttpContent stream.
The feature needs to be enabled via HttpRequestOptionsKey<bool>("WebAssemblyEnableStreamingRequest"). A helper could be added to WebAssemblyHttpRequestMessageExtensions.
It is only supported using HTTP/2 or higher and is currently only implemented in Chromium (Chrome / Edge) (see #36634 (comment)).

It currently tries to get the buffer size from the browser via desiredSize which in my testing was always 0. We can also remove this detection and just use the fixed default buffer size.
I also enabled autoAllocateChunkSize which provides us a byobRequest so we can copy directly into the browsers request buffer. Unfortunately WebAssembly/design#1162 is not yet implemented which means we can't do a zero-byte transfer by passing the ArrayBuffer to .NET and avoid any buffers in .NET.
The default buffer size is currently set to 65536. Maybe it should also be made configurable via a HttpRequestOptionsKey?

Exceptions from reading the HttpContent stream will fail the request with the thrown exception.
The special case for StringContent is still prefert and bypasses streaming.

I added some BrowserHttpHandler_* tests to ResponseStreamTest.cs analog to the WebAssemblyEnableStreamingResponse one. Maybe these BrowserHttpHandler_* test should be moved in their own file?
I duplicated the PostAsync_ThrowFromContentCopy_RequsetFails tests to use remote server, as the Http2LoopbackServer does not work in the browser.

Do I need to adjust something regarding FEATURE_WASM_THREADS?

Fixes ##36634

Author:	campersau
Assignees:	-
Labels:	`area-System.Net.Http`, `community-contribution`
Milestone:	-

pavelsavara

@campersau thanks for doing this! I really appreciate it.

I left few initial comments. Overall it seems sound implementation to me.

Do we have test for both streaming request and streaming response on same call ?
Do we dispose all the JSObject we transmit ?
Could the pull callback become static ? (so that it doesn't allocate for Delegate)

Regarding FEATURE_WASM_THREADS, I think you don't have to do anything about it and it would already work.
I plan to re-write it once more for MT, because I would like to dispatch the calls more asynchronously. Currently it's synchronously blocking the caller, which is not great. And to be able to do that I will have to do more shuffling with the way how pointers/buffers are pinned (on caller stack).
I don't know details yet. But overall it means I will do it for your new code as well.

src/mono/wasm/runtime/http.ts

campersau · 2023-08-30T15:28:03Z

Do we have test for both streaming request and streaming response on same call ?

Unit test which doesn't have all the data ready and not return synchronously would help.

I added a test which does async request and response streaming in 487ef33

Do we dispose all the JSObject we transmit ?

JSObject controller now gets disposed in e2193e6

Could the pull callback become static ? (so that it doesn't allocate for Delegate)

I made it static and moved all the state to a separate class which is send to JS and then back to .NET in 92ae132

This reverts commit 1ebb07d.

src/mono/wasm/runtime/http.ts

src/libraries/System.Net.Http/src/System/Net/Http/BrowserHttpHandler/BrowserHttpHandler.cs

src/libraries/Common/tests/System/Net/Http/ResponseStreamTest.cs

pavelsavara · 2023-08-31T14:13:42Z

I like it as it is now, @maraf could you please review it ?

pavelsavara · 2023-08-31T14:14:41Z

/azp run runtime-wasm

azure-pipelines · 2023-08-31T14:14:51Z

Azure Pipelines successfully started running 1 pipeline(s).

maraf

Great job!

src/mono/wasm/runtime/http.ts

pavelsavara · 2023-09-01T10:41:16Z

I would like to see BrowserHttpHandler_Streaming running on CI, but I can't find it in the list yet.
I would also expect it to fail on NodeJS, so I think it probably didn't run ?

campersau · 2023-09-01T11:04:31Z

I would also expect it to fail on NodeJS, so I think it probably didn't run ?

The BrowserHttpHandler_* tests only run on PlatformDetection.IsBrowser which I think does not include NodeJS, right?
Does TestPlatforms.Browser include NodeJS? Then I guess PostAsync_CancelDuringRequestContentSend_TaskCanceledQuickly could have run and failed, because we don't support streaming there yet?

pavelsavara · 2023-09-01T11:38:28Z

PlatformDetection.IsBrowser means "browser OS" and Nodejs is consuming that runtime flavor.

We have plenty of things similar to
[ActiveIssue("https://github.com/dotnet/runtime/issues/86317", typeof(PlatformDetection), nameof(PlatformDetection.IsNodeJS))] which do work on actual browser.

Also [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotNodeJS))]

campersau · 2023-09-01T13:05:49Z

I think it is because of this:

runtime/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs

Lines 643 to 647 in f470200

    
           [ConditionalClass(typeof(PlatformDetection), nameof(PlatformDetection.IsBrowserDomSupportedOrNotBrowser))] 
        
           public sealed class SocketsHttpHandler_ResponseStreamTest : ResponseStreamTest 
        
           { 
        
               public SocketsHttpHandler_ResponseStreamTest(ITestOutputHelper output) : base(output) { } 
        
           }

If I remove that attribute and run

./dotnet.sh build /t:test /p:TargetOS=browser /p:TargetArchitecture=wasm /p:Scenario=WasmTestOnNodeJS /p:Configuration=Release src/libraries/System.Net.Http/tests/FunctionalTests/System.Net.Http.Functional.Tests.csproj /p:xunitoptions="-method System.Net.Http.Functional.Tests.SocketsHttpHandler_ResponseStreamTest.BrowserHttpHandler_Streaming"

I get failure, but I am not sure why it is this one..

  info: [STRT] System.Net.Http.Functional.Tests.SocketsHttpHandler_ResponseStreamTest.BrowserHttpHandler_Streaming
  fail: [FAIL] System.Net.Http.Functional.Tests.SocketsHttpHandler_ResponseStreamTest.BrowserHttpHandler_Streaming
  info: System.Net.Http.HttpRequestException : Error while copying content to a stream.
  info: ---- System.ObjectDisposedException : Cannot access a closed Stream.
  info: Object name: 'DelegateStream'.
  info:    at System.Net.Http.HttpContent.LoadIntoBufferAsyncCore(Task serializeToStreamTask, MemoryStream tempBuffer)
  info:    at System.Net.Http.HttpContent.<WaitAndReturnAsync>d__82`2[[System.Net.Http.HttpContent, System.Net.Http, Version=8.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a],[System.Byte[], System.Private.CoreLib, Version=8.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].MoveNext()
  info:    at System.Net.Http.BrowserHttpHandler.CallFetch(HttpRequestMessage request, CancellationToken cancellationToken, Nullable`1 allowAutoRedirect)
  info:    at System.Net.Http.BrowserHttpHandler.<SendAsync>g__Impl|55_0(HttpRequestMessage request, CancellationToken cancellationToken, Nullable`1 allowAutoRedirect)
  info:    at System.Net.Http.HttpClient.<SendAsync>g__Core|83_0(HttpRequestMessage request, HttpCompletionOption completionOption, CancellationTokenSource cts, Boolean disposeCts, CancellationTokenSource pendingRequestsCts, CancellationToken originalCancellationToken)
  info:    at System.Net.Http.Functional.Tests.ResponseStreamTest.BrowserHttpHandler_Streaming()
  info: --- End of stack trace from previous location ---
  info: ----- Inner Stack Trace -----
  info:    at System.IO.Stream.CopyToAsync(Stream destination, Int32 bufferSize, CancellationToken cancellationToken)
  info:    at System.IO.Stream.CopyToAsync(Stream destination, CancellationToken cancellationToken)
  info:    at System.Net.Http.StreamToStreamCopy.CopyAsync(Stream source, Stream destination, Int32 bufferSize, Boolean disposeSource, CancellationToken cancellationToken)
  info: --- End of stack trace from previous location ---
  info:    at System.Net.Http.HttpContent.LoadIntoBufferAsyncCore(Task serializeToStreamTask, MemoryStream tempBuffer)

pavelsavara · 2023-09-01T13:06:56Z

This is chrome
[PASS] System.Net.Http.Functional.Tests.SocketsHttpHandler_ResponseStreamTest.BrowserHttpHandler_Streaming

pavelsavara · 2023-09-01T13:15:55Z

This is NodeJS

It doesn't run the test because

runtime/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs

Line 643 in f470200

    
           [ConditionalClass(typeof(PlatformDetection), nameof(PlatformDetection.IsBrowserDomSupportedOrNotBrowser))]

Which is OK

guardrex · 2023-09-01T13:33:50Z

@pavelsavara ... The issue and PR are marked for 9.0 milestone. I just want to confirm that this will indeed be for 9.0.

pavelsavara · 2023-09-01T13:40:49Z

I didn't plan to backport this to Net8, it's too late for that I think.

@lewing your expectations ?

matthewreiter · 2023-09-05T16:10:41Z

I noticed that the implementation uses request.Content.ReadAsStreamAsync, which will result in buffering the entire request body when uploading multipart content where one of the parts comes from the InputFile component.

For example:

<InputFile OnChange="OnFileSelect" />
@code {
    void OnFileSelect(InputFileChangeEventArgs e) {
        IBrowserFile file = e.File;
        using var fileStream = file.OpenReadStream(long.MaxValue);
        using var stringContent = new StringContent("Test");
        using var fileContent = new StreamContent(fileStream);

        using var multipartContent = new MultipartFormDataContent
        {
            { stringContent, "metadata" },
            { fileContent, "file", file.Name },
        };

        // And then do a post with multipartContent
}

The issue is that the stream produced by IBrowserFile.OpenReadStream isn't seekable, and MultipartContent.CreateContentReadStreamAsyncCore (indirectly called by ReadAsStreamAsync) will buffer the content in a MemoryStream if any of the streams aren't seekable. In my implementation of streaming requests (which I literally wrote a week and a half ago because I couldn't wait for it to get into the runtime), I resorted to using request.Content.CopyToAsync into a Pipe instead of using request.Content.ReadAsStreamAsync in order to get around this limitation, but a better solution would probably be to change the way MultipartContent.CreateContentReadStreamAsyncCore works to avoid the buffering.

pavelsavara · 2023-09-05T17:59:06Z

a better solution would probably be to change the way MultipartContent.CreateContentReadStreamAsyncCore works to avoid the buffering.

@greenEkatherine, @CarnaViire any thought on the feedback above ?
Should we create new issue for that ?
Does the proposed fix make sense for all platforms or we should seek WASM specific fix ?

campersau · 2023-09-05T22:40:02Z

We could change the implementation to use HttpContent.CopyToAsync instead and use a TransformStream in JS which could look something like this: campersau@963f9ff

campersau added 6 commits August 27, 2023 21:36

progress

d94e62d

progress try out byob

c52325d

back to enqueue again

a85a8c8

finalize request stream test

ec46048

checkpoint

d9e12ce

reject fetch with read stream exception

5297b09

campersau requested review from lewing, pavelsavara and kg as code owners August 29, 2023 22:38

ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 29, 2023

dotnet-issue-labeler bot added the area-System.Net.Http label Aug 29, 2023

lewing added this to the 9.0.0 milestone Aug 30, 2023

lewing assigned pavelsavara Aug 30, 2023

lewing requested a review from maraf August 30, 2023 01:14

build-analysis bot mentioned this pull request Aug 30, 2023

Tracking issue for CI build timeouts #76454

Closed

fix build

df3b0c9

campersau force-pushed the browserrequeststreaming branch from 8a16719 to df3b0c9 Compare August 30, 2023 06:28

build-analysis bot mentioned this pull request Aug 30, 2023

STATUS_UNSUCCESSFUL in RsaCryptRoundtrip_OaepSHA1 #29683

Open

pavelsavara reviewed Aug 30, 2023

View reviewed changes

src/mono/wasm/runtime/http.ts Outdated Show resolved Hide resolved

src/mono/wasm/runtime/http.ts Outdated Show resolved Hide resolved

src/mono/wasm/runtime/http.ts Outdated Show resolved Hide resolved

campersau added 4 commits August 30, 2023 14:47

use createPromiseController in pull

cdfeab2

dispose controller

e2193e6

swap delegate allocation with state allocation

92ae132

add response / request streaming test

487ef33

campersau added 2 commits August 30, 2023 19:24

use createPromiseController for fetch

1ebb07d

Revert "use createPromiseController for fetch"

2fce6de

This reverts commit 1ebb07d.

pavelsavara reviewed Aug 30, 2023

View reviewed changes

src/mono/wasm/runtime/http.ts Outdated Show resolved Hide resolved

campersau added 2 commits August 30, 2023 20:29

rename pull to pull_delegate

cd4957a

move reading into ReadableStreamPullState

a69fd0a

call ReadableStreamControllerEnqueueUnsafe again

dad30c5

pavelsavara reviewed Aug 31, 2023

View reviewed changes

src/libraries/System.Net.Http/src/System/Net/Http/BrowserHttpHandler/BrowserHttpHandler.cs Show resolved Hide resolved

pavelsavara reviewed Aug 31, 2023

View reviewed changes

src/libraries/Common/tests/System/Net/Http/ResponseStreamTest.cs Outdated Show resolved Hide resolved

BrowserHttpHandler_Streaming not OuterLoop

ee3a4a7

This was referenced Aug 31, 2023

LibraryImportGenerator unit test segfault illegal memory access #89054

Closed

[browser] mono runtime already exited with 0 #89425

Closed

[browser] Fragile WebSocket CI tests #90257

Closed

JSMarshalAsAttribute failing error tests #91410

Closed

maraf approved these changes Sep 1, 2023

View reviewed changes

src/mono/wasm/runtime/http.ts Show resolved Hide resolved

add comment for streaming request feature detection logic

a594c92

pavelsavara merged commit 9e16f09 into dotnet:main Sep 1, 2023
114 of 116 checks passed

campersau deleted the browserrequeststreaming branch September 1, 2023 13:17

guardrex mentioned this pull request Sep 1, 2023

Blazor project tracking 2023 dotnet/AspNetCore.Docs#28001

Closed

campersau mentioned this pull request Sep 1, 2023

Add SetBrowserRequestStreamingEnabled extension method dotnet/aspnetcore#50481

Merged

campersau mentioned this pull request Sep 6, 2023

[browser] [wasm] Refactor Request Streaming to use HttpContent.CopyToAsync #91699

Merged

ghost locked as resolved and limited conversation to collaborators Oct 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[browser] [wasm] Request Streaming upload #91295

[browser] [wasm] Request Streaming upload #91295

campersau commented Aug 29, 2023 •

edited

Loading

ghost commented Aug 29, 2023

pavelsavara left a comment •

edited

Loading

campersau commented Aug 30, 2023 •

edited

Loading

pavelsavara commented Aug 31, 2023

pavelsavara commented Aug 31, 2023

azure-pipelines bot commented Aug 31, 2023

maraf left a comment

pavelsavara commented Sep 1, 2023

campersau commented Sep 1, 2023 •

edited

Loading

pavelsavara commented Sep 1, 2023 •

edited

Loading

campersau commented Sep 1, 2023

pavelsavara commented Sep 1, 2023

pavelsavara commented Sep 1, 2023

guardrex commented Sep 1, 2023

pavelsavara commented Sep 1, 2023

matthewreiter commented Sep 5, 2023 •

edited

Loading

pavelsavara commented Sep 5, 2023 •

edited

Loading

campersau commented Sep 5, 2023

[browser] [wasm] Request Streaming upload #91295

[browser] [wasm] Request Streaming upload #91295

Conversation

campersau commented Aug 29, 2023 • edited Loading

ghost commented Aug 29, 2023

pavelsavara left a comment • edited Loading

Choose a reason for hiding this comment

campersau commented Aug 30, 2023 • edited Loading

pavelsavara commented Aug 31, 2023

pavelsavara commented Aug 31, 2023

azure-pipelines bot commented Aug 31, 2023

maraf left a comment

Choose a reason for hiding this comment

pavelsavara commented Sep 1, 2023

campersau commented Sep 1, 2023 • edited Loading

pavelsavara commented Sep 1, 2023 • edited Loading

campersau commented Sep 1, 2023

pavelsavara commented Sep 1, 2023

pavelsavara commented Sep 1, 2023

guardrex commented Sep 1, 2023

pavelsavara commented Sep 1, 2023

matthewreiter commented Sep 5, 2023 • edited Loading

pavelsavara commented Sep 5, 2023 • edited Loading

campersau commented Sep 5, 2023

campersau commented Aug 29, 2023 •

edited

Loading

pavelsavara left a comment •

edited

Loading

campersau commented Aug 30, 2023 •

edited

Loading

campersau commented Sep 1, 2023 •

edited

Loading

pavelsavara commented Sep 1, 2023 •

edited

Loading

matthewreiter commented Sep 5, 2023 •

edited

Loading

pavelsavara commented Sep 5, 2023 •

edited

Loading