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

ReadableStream: Add a test that all queued writes complete when closing after first write #9573

Conversation

Projects
None yet
5 participants
@MattiasBuelens
Copy link
Contributor

MattiasBuelens commented Feb 19, 2018

This PR adds more tests for the new behavior introduced in whatwg/streams#884, namely that all queued writes must complete before a pipe operation becomes completed.

When the readable stream closes, the pipe operation should wait for all writes to complete, then close the writable stream (only if preventClose != true), and then resolve the pipe operation's promise. The new tests check the handling of both preventClose = true and preventClose != true, although the latter case was already handled correctly by the reference implementation.

The new tests are very similar to the one added in #5636, which verifies similar behavior for when the readable stream becomes errored.

ReadableStream: Add a test that all queued writes complete when closi…
…ng after first write and preventClose = true

When the readable stream becomes closed after the first write,
all queued writes should complete before the pipe operation
becomes completed.

This tests the fix for this behaviour in the Streams reference
implementation in whatwg/streams#884.
@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Feb 19, 2018

Build PASSED

Started: 2018-02-19 22:19:54
Finished: 2018-02-19 22:40:09

View more information about this build on:

@MattiasBuelens

This comment has been minimized.

Copy link
Contributor Author

MattiasBuelens commented Feb 19, 2018

It seems like the cases where preventClose != true are already handled correctly, although it's not very obvious why. The pipe operation calls WritableStreamDefaultWriterCloseWithErrorPropagation and awaits the returned promise, which resolves when the close request completes. The close request is queued after any other writes, so it will only complete after those writes are complete. In principle, the pipe operation could skip waiting for the queued writes to complete and instead just await the promise from the close request, but I wouldn't recommend doing this since this isn't very obvious. 😛

For completeness sake, I'll add another test for the case where preventClose != true, but existing implementations should already be handling this correctly.

ReadableStream: Add a test that all queued writes complete when closi…
…ng after first write without preventClose = true

@MattiasBuelens MattiasBuelens changed the title ReadableStream: Add a test that all queued writes complete when closing after first write and preventClose = true ReadableStream: Add a test that all queued writes complete when closing after first write Feb 19, 2018

@ricea

ricea approved these changes Feb 20, 2018

Copy link
Contributor

ricea left a comment

lgtm

@domenic domenic merged commit 4c74a92 into web-platform-tests:master Feb 20, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MattiasBuelens MattiasBuelens deleted the MattiasBuelens:readable-stream-pipeto-close-propagation-forward-queued-writes-prevent-close branch Feb 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.