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.

Sign up for GitHub

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

Jump to bottom

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

Merged

pavelsavara merged 15 commits into dotnet:main from campersau:browserrequeststreamtransform

Sep 21, 2023

Contributor

campersau commented Sep 6, 2023

Uses a TransformStream into which the HttpContent gets copied into. Sync writes are not allowed like sync reads in response streaming.

Ref: #91295 (comment)


          try out transformstream

36f5f5a

campersau requested review from lewing, pavelsavara and kg as code owners

September 6, 2023 22:23

dotnet-issue-labeler bot added the area-System.Net.Http label

ghost added the community-contribution label

ghost commented Sep 6, 2023

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

Issue Details

Uses a TransformStream into which the HttpContent gets copied into. Sync writes are not allowed like sync reads in response streaming.

Ref: #91295 (comment)

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

This was referenced Sep 7, 2023

Tracking issue for CI build timeouts #76454

Closed

Networking certificate test failures #91705

Closed

pavelsavara reviewed

View reviewed changes

Member

pavelsavara left a comment

Nice work thanks!
I wonder if the transformer could also improve response streaming :)

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

src/mono/wasm/runtime/http.ts Outdated

+                  // the bufferPtr is pinned by the caller
+                  const view = new Span(bufferPtr, bufferLength, MemoryViewType.Byte);
+                  const copy = view.slice() as Uint8Array;
+                  await ts.__writer.write(copy);

Member

pavelsavara Sep 7, 2023

I would love this to be cancellable.

Member

pavelsavara Sep 7, 2023

I assume this may throw when

TPC breaks or server kills the connection mid flight
we triggered AbortController

Contributor Author

campersau Sep 7, 2023 •

edited

Loading

Unfortunately not, I need to race all of these with the fetch_promise_control e.g.

await Promise.race([ts.__writer.write(copy), ts.__fetch_promise_control.promise]);

or do you have another idea?

var t = new TransformStream();
var w = t.writable.getWriter();
var a = new AbortController();

var f = fetch("", {
    method: "POST",
    body: t.readable,
    duplex: "half",
    signal: a.signal
});

a.abort();

var wp = w.write(new Uint8Array([1, 2, 3]));

var done = Promise.race([wp, f]);

// wp is pending and never settles
// done / f is rejected

Contributor Author

campersau Sep 8, 2023

I opened a bug report in chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=1480250
Because I would expect that any fetch error would properly cancel the ReadableStream (which the browser locks) and thus any writer promises would be rejected.

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

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


          PR feedback

7ecb8d6

build-analysis bot mentioned this pull request

CI error: System.Net.Quic.QuicException: The connection timed out from inactivity #91757

Closed


          race transform stream writing with fetch promise

03491b4

unfortunately the transform stream does not get notified when fetch fails or cancels so we need to race with it

campersau commented

View reviewed changes

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

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

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

This was referenced Sep 8, 2023

[wasm][debugger] Debugger tests getting randomly aborted on CI #89409

Open

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

campersau added 3 commits

September 8, 2023 13:19


          add more tests for http1 failed fetch and cancellation

d895743


          PR feedback

73d5289


          test more return types for request cancellation

26dd68a

pavelsavara reviewed

View reviewed changes

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

pavelsavara reviewed

View reviewed changes

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

pavelsavara reviewed

View reviewed changes

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

pavelsavara reviewed

View reviewed changes

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

campersau added 2 commits

September 8, 2023 14:40


          PR feedback wrap_as_cancelable_promise

5480c03


          PR feedback rename to fetchPromise

c4601b9

build-analysis bot mentioned this pull request

[debugger] SteppingTests fail on CI with Command timed out, and was killed #91801

Closed


          test async cancel

330d1ba

pavelsavara reviewed

View reviewed changes

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

pavelsavara reviewed

View reviewed changes

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


          use CancelationHelper

f2a7f9d

campersau force-pushed the browserrequeststreamtransform branch from afd05d7 to f2a7f9d Compare

September 8, 2023 20:01


          throw from readFunc to make sure it isn't called

20130a6

build-analysis bot mentioned this pull request

System.Data.OleDb.Tests timeout in net48 x86 Release leg #87783

Open

campersau mentioned this pull request

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

Merged


          use s instead of state for consistency

89f2b86

campersau commented

View reviewed changes

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

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

karelz added this to the 9.0.0 milestone

Member

pavelsavara commented Sep 14, 2023

I guess the ball is on my side of the court now, I will do full review early next week. Sorry for delay.


          improve exception handling

a11b798

pavelsavara reviewed

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

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

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

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

marek-safar added the arch-wasm label

ghost commented Sep 20, 2023

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

Issue Details

Uses a TransformStream into which the HttpContent gets copied into. Sync writes are not allowed like sync reads in response streaming.

Ref: #91295 (comment)

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

campersau added 2 commits

September 20, 2023 16:09


          return ControllablePromise

06b7f69


          dispatch TransformStreamWrite on FEATURE_WASM_THREADS

0f2ee8f

Member

pavelsavara commented Sep 20, 2023

/azp run runtime-wasm

azure-pipelines bot commented Sep 20, 2023

Azure Pipelines successfully started running 1 pipeline(s).

campersau mentioned this pull request

[browser] move files around #92429

Closed

pavelsavara approved these changes

View reviewed changes

pavelsavara merged commit 4ad310d into dotnet:main

124 of 127 checks passed

campersau deleted the browserrequeststreamtransform branch

September 21, 2023 20:14

ghost locked as resolved and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

arch-wasm area-System.Net.Http community-contribution