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

Adapt to changes in Streams #1044

Merged
merged 4 commits into from Jul 7, 2020
Merged

Adapt to changes in Streams #1044

merged 4 commits into from Jul 7, 2020

Conversation

domenic
Copy link
Member

@domenic domenic commented Jun 26, 2020

Fixes #780. Fixes #976. Follows whatwg/streams#1045.

(At least, I think this fixes #976. That's a long issue with lots of text, but this significantly reduces the amount of "in parallel" that happens in upload streaming, so it seems likely to address the problem.)

This should land after whatwg/streams#1045 and needs some updates once it does, so tagging "do not merge yet". But it's ready for review.

The "looping" algorithms could potentially use better names; bikeshed help welcome.


Preview | Diff

@domenic domenic added the do not merge yet Pull request must not be merged per rationale in comment label Jun 26, 2020
domenic added a commit to whatwg/streams that referenced this pull request Jun 26, 2020
Although the long-term relationship isn't obvious, whatwg/fetch#1044 wants this at least for now
Copy link
Collaborator

@ricea ricea left a comment

Choose a reason for hiding this comment

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

lgtm

I don't have any better ideas for how to phrase the looping constructs.

fetch.bs Outdated
Comment on lines 1718 to 1719
<li><p>If the ongoing fetch is not <a for=fetch>terminated</a>, then perform the
<a>transmit-body loop</a> given <var>request</var>, <var>body</var>, and <var>reader</var>.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems like cheating. Don't we have to post a task to get back out of "in parallel"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, oops, I meant to do that.

@annevk annevk requested a review from yutakahirano June 29, 2020 14:50
@domenic domenic removed the do not merge yet Pull request must not be merged per rationale in comment label Jul 6, 2020
@domenic
Copy link
Member Author

domenic commented Jul 6, 2020

The corresponding streams PR has been merged, although we need to wait 24 hours or so before the linking database updates and we can remove the whatpr.org links.

@domenic domenic added the do not merge yet Pull request must not be merged per rationale in comment label Jul 6, 2020
@domenic domenic removed the do not merge yet Pull request must not be merged per rationale in comment label Jul 7, 2020
@domenic
Copy link
Member Author

domenic commented Jul 7, 2020

This is ready to merge, @annevk.

@annevk annevk merged commit 801dc8b into master Jul 7, 2020
@annevk annevk deleted the update-streams-usage branch July 7, 2020 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants