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

Use tasks to manipulate streams #1270

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Use tasks to manipulate streams #1270

wants to merge 1 commit into from

Conversation

annevk
Copy link
Member

@annevk annevk commented Jul 20, 2021

Helps with #1246.

The main problem with this PR (which is why I marked it WIP) is that the stream itself is created in parallel and I don't see a good way to avoid that. If we had low-level streams this would be different.

  • At least two implementers are interested (and none opposed):
  • Tests are written and can be reviewed and commented upon at:
  • Implementation bugs are filed:
    • Chrome: …
    • Firefox: …
    • Safari: …

(See WHATWG Working Mode: Changes for more details.)


Preview | Diff

Helps with #1246.

The main problem with this PR (which is why I marked it WIP) is that the stream itself is created in parallel and I don't see a good way to avoid that. If we had low-level streams this would be different.
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Is the stream created in parallel? It's created in the first portion of HTTP-network fetch, but maybe HTTP-network fetch itself was called in parallel?

Another architecture which you could explore is to do everything in the main thread except for the "wait for bytes or finished signal from the network" step, and then queue a task whenever you get bytes (regardless of whether it's bytes or a finished signal). I.e., spend the minimum amount of time possible in parallel.

<a lt=terminated for=fetch>terminate</a> the ongoing fetch.
<ol>
<li><p><a for=ReadableStream>Enqueue</a> a {{Uint8Array}} wrapping an {{ArrayBuffer}}
containing <var>bytes</var> into <var>stream</var>.
Copy link
Member

Choose a reason for hiding this comment

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

FWIW this can be made rigorous these days with https://heycam.github.io/webidl/#arraybufferview-create ([=ArrayBufferView/new=] {{Uint8Array}} given |bytes| in some realm).

<li><p>Otherwise, if the bytes transmission for <var>response</var>'s message body is done
normally and <var>stream</var> is <a for=ReadableStream>readable</a>, then
<a for=ReadableStream>close</a> <var>stream</var>, <a for=/>finalize response</a> for
<var>fetchParams</var> and <var>response</var>.
Copy link
Member

Choose a reason for hiding this comment

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

Closing should also be done in a task right?

@annevk
Copy link
Member Author

annevk commented Jul 23, 2021

Yeah, after main fetch everything is in parallel. I think it really shows that JavaScript streams are just not that suitable for this. And even if we used an implementation-defined realm we'd still have to do everything in the same in parallel thread somehow, which we don't really have a mechanism for.

@domenic
Copy link
Member

domenic commented Jul 23, 2021

Ok. What do you think of my alternate model suggestion to address that?

@annevk
Copy link
Member Author

annevk commented Jul 23, 2021

I'm not sure I understand it. How could we stay in the main thread?

@domenic
Copy link
Member

domenic commented Jul 23, 2021

Only go in parallel for the steps that actually receive bytes from the network, since those are the only steps that need to block on some external action. Then immediately queue a task back to the main thread after you're done getting those bytes.

@annevk
Copy link
Member Author

annevk commented Jul 29, 2021

That would mean you'd always have to invoke fetch from an event loop thread, right? I'm not sure that works for Background Fetch. And it would also require restructuring other existing callers, though that is needed either way to be fair.

And it would mean you'd have to queue a task to resume the fetch algorithm (to deal with redirects and such) which I guess could work, but it seems like a pretty large undertaking.

@domenic
Copy link
Member

domenic commented Jul 29, 2021

Yeah, it's definitely a large undertaking.

I agree it doesn't work for background fetch; you'd need some sort of special casing for that. (Or a fake event loop, eugh.)

It might be best to take a step back and ask how implementations handle this. Maybe we could model that more closely. Presumably they create any ReadableStream objects only (?) for fetch() calls or service worker Request/Response construction, on the main thread. And then queue tasks to manipulate them as appropriate.

@whatwg whatwg deleted a comment from triwit7417 Jul 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants