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

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

Merged
merged 15 commits into from
Sep 21, 2023

Conversation

campersau
Copy link
Contributor

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

Ref: #91295 (comment)

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Sep 6, 2023
@ghost
Copy link

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: -

Copy link
Member

@pavelsavara pavelsavara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love this to be cancellable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this may throw when

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

Copy link
Contributor Author

@campersau campersau Sep 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

unfortunately the transform stream does not get notified when fetch fails or cancels so we need to race with it
@campersau campersau force-pushed the browserrequeststreamtransform branch from afd05d7 to f2a7f9d Compare September 8, 2023 20:01
@karelz karelz added this to the 9.0.0 milestone Sep 12, 2023
@pavelsavara
Copy link
Member

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

@marek-safar marek-safar added the arch-wasm WebAssembly architecture label Sep 20, 2023
@ghost
Copy link

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

@pavelsavara
Copy link
Member

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara pavelsavara merged commit 4ad310d into dotnet:main Sep 21, 2023
124 of 127 checks passed
@campersau campersau deleted the browserrequeststreamtransform branch September 21, 2023 20:14
@ghost ghost locked as resolved and limited conversation to collaborators Oct 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-System.Net.Http 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.

4 participants