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

Set up ReceiveStream with proper algorithms #297

Merged
merged 15 commits into from
Jul 30, 2021
Merged

Conversation

yutakahirano
Copy link
Contributor

@yutakahirano yutakahirano commented Jun 28, 2021

Define pullAlgorithm and cancelAlgorithm, and set up ReceiveStream
with them.

This relies on whatwg/streams#1130.

Fixes #185.


💥 Error: 500 Internal Server Error 💥

PR Preview failed to build. (Last tried on Jul 29, 2021, 2:42 PM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 CSS Spec Preprocessor - CSS Spec Preprocessor is the web service used to build Bikeshed specs.

🔗 Related URL

<html><body><p><strong>WARNING: </strong>mysqli_connect(): (HY000/2002): Can't connect to local MySQL server through socket '/var/run/mysqld/mysqld.sock' (111)</p></body></html>

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

Define pullAlgorithm and cancelAlgorithm, and set up ReceiveStream
with them.

This relies on whatwg/streams#1130.

Fixes #185.
@yutakahirano
Copy link
Contributor Author

@ricea, can you take a look?
@domenic for the interaction with whatwg/streams#1130: Am I using it correctly?

@yutakahirano yutakahirano requested a review from ricea June 28, 2021 09:35
Copy link
Contributor

@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.

I think we should wait for whatwg/streams#1130.

1. If |hasReceivedFIN| is true:
1. [=set/Remove=] |stream| from |transport|'s [=[[ReceiveStreams]]=].
1. [=ReadableStream/Close=] |stream|.
1. [=Resolve=] |promise| with undefined.
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like if a RESET_STREAM signal comes from the server, this promise will never be resolved. It doesn't seem to be observable, but it feels untidy to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a step and a note. We need to reject the promise with a proper error object, so I'd like to leave it as is.

@domenic
Copy link

domenic commented Jul 15, 2021

The Streams spec change has been merged and within 24 hours should show up in cross-linking databases. One thing that changed is we used "enqueue" instead of "enqueue bytes".

@yutakahirano
Copy link
Contributor Author

Thank you! I updated the PR. PTAL again.

Copy link

@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.

Looks great. Thank you for the helpful back and forth, and your patience along the way!

@yutakahirano
Copy link
Contributor Author

@ricea, can you take a look again?

Copy link
Contributor

@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 with nits.

yutakahirano and others added 3 commits July 29, 2021 23:13
@ricea
Copy link
Contributor

ricea commented Jul 29, 2021

lgtm

@yutakahirano
Copy link
Contributor Author

Thank you!

@yutakahirano yutakahirano merged commit d1be624 into main Jul 30, 2021
@yutakahirano yutakahirano deleted the yhirano/receive-stream branch July 30, 2021 02:48
github-actions bot added a commit that referenced this pull request Jul 30, 2021
SHA: d1be624
Reason: push, by @yutakahirano

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Call the streams algorithms
3 participants