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.

Sign up for GitHub

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

Jump to bottom

streams: pipeTo a WS with HWM>1 #5262

Merged
merged 2 commits into from
Mar 31, 2017
Merged

streams: pipeTo a WS with HWM>1 #5262

merged 2 commits into from
Mar 31, 2017

Conversation

isonmad
Copy link
Contributor

@isonmad isonmad commented Mar 29, 2017 •
edited
Loading

pipeTo in the reference implementation currently effectively ignores the WritableStream's highWaterMark. Or is that intended behavior?

@wpt-pr-bot
Copy link
Collaborator

Notifying @calvaris, @domenic, @ricea, @tyoshino, @wanderview, @youennf, and @yutakahirano. (Learn how reviewing works.)

@ghost
Copy link

ghost commented Mar 29, 2017 •
edited by ghost
Loading

View the complete job log.

Firefox (nightly channel)

Testing web-platform-tests at revision 5430c5b
Using browser at version BuildID 20170329100319; SourceStamp 272ce6c2572164f5f6a9fba2a980ba9ccf50770c
Starting 10 test iterations
All results were stable

All results

6 tests ran
/console/console-timeline-timelineEnd-historical.any.html
Subtest Results Messages
OK
'timeline' function should not exist on the console object FAIL assert_equals: console.timeline should be undefined expected (undefined) undefined but got (function) function "function timeline() {\n [native code]\n}"
'timelineEnd' function should not exist on the console object FAIL assert_equals: console.timelineEnd should be undefined expected (undefined) undefined but got (function) function "function timelineEnd() {\n [native code]\n}"
/console/console-timeline-timelineEnd-historical.any.worker.html
Subtest Results Messages
OK
'timeline' function should not exist on the console object FAIL assert_equals: console.timeline should be undefined expected (undefined) undefined but got (function) function "function timeline() {\n [native code]\n}"
'timelineEnd' function should not exist on the console object FAIL assert_equals: console.timelineEnd should be undefined expected (undefined) undefined but got (function) function "function timelineEnd() {\n [native code]\n}"
/streams/piping/flow-control.dedicatedworker.html
Subtest Results Messages
OK
Piping from a non-empty ReadableStream into a WritableStream that does not desire chunks FAIL ReadableStream is not defined
Piping from a non-empty ReadableStream into a WritableStream that does not desire chunks, but then does FAIL ReadableStream is not defined
Piping from an empty ReadableStream into a WritableStream that does not desire chunks, but then the readable stream becomes non-empty and the writable stream starts desiring chunks FAIL ReadableStream is not defined
Piping from a ReadableStream to a WritableStream that desires more chunks before finishing with previous ones FAIL CountQueuingStrategy is not defined
Piping to a WritableStream that does not consume the writes fast enough exerts backpressure on the ReadableStream FAIL ReadableStream is not defined
/streams/piping/flow-control.html
Subtest Results Messages
OK
Piping from a non-empty ReadableStream into a WritableStream that does not desire chunks FAIL ReadableStream is not defined
Piping from a non-empty ReadableStream into a WritableStream that does not desire chunks, but then does FAIL ReadableStream is not defined
Piping from an empty ReadableStream into a WritableStream that does not desire chunks, but then the readable stream becomes non-empty and the writable stream starts desiring chunks FAIL ReadableStream is not defined
Piping from a ReadableStream to a WritableStream that desires more chunks before finishing with previous ones FAIL CountQueuingStrategy is not defined
Piping to a WritableStream that does not consume the writes fast enough exerts backpressure on the ReadableStream FAIL ReadableStream is not defined
/streams/piping/flow-control.serviceworker.https.html
Subtest Results Messages
OK
Service worker test setup PASS
Piping from a non-empty ReadableStream into a WritableStream that does not desire chunks FAIL ReadableStream is not defined
Piping from a non-empty ReadableStream into a WritableStream that does not desire chunks, but then does FAIL ReadableStream is not defined
Piping from an empty ReadableStream into a WritableStream that does not desire chunks, but then the readable stream becomes non-empty and the writable stream starts desiring chunks FAIL ReadableStream is not defined
Piping from a ReadableStream to a WritableStream that desires more chunks before finishing with previous ones FAIL CountQueuingStrategy is not defined
Piping to a WritableStream that does not consume the writes fast enough exerts backpressure on the ReadableStream FAIL ReadableStream is not defined
/streams/piping/flow-control.sharedworker.html
Subtest Results Messages
OK
Piping from a non-empty ReadableStream into a WritableStream that does not desire chunks FAIL ReadableStream is not defined
Piping from a non-empty ReadableStream into a WritableStream that does not desire chunks, but then does FAIL ReadableStream is not defined
Piping from an empty ReadableStream into a WritableStream that does not desire chunks, but then the readable stream becomes non-empty and the writable stream starts desiring chunks FAIL ReadableStream is not defined
Piping from a ReadableStream to a WritableStream that desires more chunks before finishing with previous ones FAIL CountQueuingStrategy is not defined
Piping to a WritableStream that does not consume the writes fast enough exerts backpressure on the ReadableStream FAIL ReadableStream is not defined

@ghost
Copy link

ghost commented Mar 29, 2017 •
edited by ghost
Loading

View the complete job log.

Chrome (unstable channel)

Testing web-platform-tests at revision 5430c5b
Using browser at version 59.0.3053.3 dev
Starting 10 test iterations
All results were stable

All results

6 tests ran
/console/console-timeline-timelineEnd-historical.any.html
Subtest Results Messages
OK
'timeline' function should not exist on the console object FAIL assert_equals: console.timeline should be undefined expected (undefined) undefined but got (function) function "function timeline() { [native code] }"
'timelineEnd' function should not exist on the console object FAIL assert_equals: console.timelineEnd should be undefined expected (undefined) undefined but got (function) function "function timelineEnd() { [native code] }"
/console/console-timeline-timelineEnd-historical.any.worker.html
Subtest Results Messages
OK
'timeline' function should not exist on the console object FAIL assert_equals: console.timeline should be undefined expected (undefined) undefined but got (function) function "function timeline() { [native code] }"
'timelineEnd' function should not exist on the console object FAIL assert_equals: console.timelineEnd should be undefined expected (undefined) undefined but got (function) function "function timelineEnd() { [native code] }"
/streams/piping/flow-control.dedicatedworker.html
Subtest Results Messages
OK
Piping from a non-empty ReadableStream into a WritableStream that does not desire chunks FAIL WritableStream is not defined
Piping from a non-empty ReadableStream into a WritableStream that does not desire chunks, but then does FAIL WritableStream is not defined
Piping from an empty ReadableStream into a WritableStream that does not desire chunks, but then the readable stream becomes non-empty and the writable stream starts desiring chunks FAIL WritableStream is not defined
Piping from a ReadableStream to a WritableStream that desires more chunks before finishing with previous ones FAIL WritableStream is not defined
Piping to a WritableStream that does not consume the writes fast enough exerts backpressure on the ReadableStream FAIL WritableStream is not defined
/streams/piping/flow-control.html
Subtest Results Messages
OK
Piping from a non-empty ReadableStream into a WritableStream that does not desire chunks FAIL WritableStream is not defined
Piping from a non-empty ReadableStream into a WritableStream that does not desire chunks, but then does FAIL WritableStream is not defined
Piping from an empty ReadableStream into a WritableStream that does not desire chunks, but then the readable stream becomes non-empty and the writable stream starts desiring chunks FAIL WritableStream is not defined
Piping from a ReadableStream to a WritableStream that desires more chunks before finishing with previous ones FAIL WritableStream is not defined
Piping to a WritableStream that does not consume the writes fast enough exerts backpressure on the ReadableStream FAIL WritableStream is not defined
/streams/piping/flow-control.serviceworker.https.html
Subtest Results Messages
OK
Service worker test setup PASS
Piping from a non-empty ReadableStream into a WritableStream that does not desire chunks FAIL WritableStream is not defined
Piping from a non-empty ReadableStream into a WritableStream that does not desire chunks, but then does FAIL WritableStream is not defined
Piping from an empty ReadableStream into a WritableStream that does not desire chunks, but then the readable stream becomes non-empty and the writable stream starts desiring chunks FAIL WritableStream is not defined
Piping from a ReadableStream to a WritableStream that desires more chunks before finishing with previous ones FAIL WritableStream is not defined
Piping to a WritableStream that does not consume the writes fast enough exerts backpressure on the ReadableStream FAIL WritableStream is not defined
/streams/piping/flow-control.sharedworker.html
Subtest Results Messages
OK
Piping from a non-empty ReadableStream into a WritableStream that does not desire chunks FAIL WritableStream is not defined
Piping from a non-empty ReadableStream into a WritableStream that does not desire chunks, but then does FAIL WritableStream is not defined
Piping from an empty ReadableStream into a WritableStream that does not desire chunks, but then the readable stream becomes non-empty and the writable stream starts desiring chunks FAIL WritableStream is not defined
Piping from a ReadableStream to a WritableStream that desires more chunks before finishing with previous ones FAIL WritableStream is not defined
Piping to a WritableStream that does not consume the writes fast enough exerts backpressure on the ReadableStream FAIL WritableStream is not defined

@domenic
Copy link
Member

domenic commented Mar 30, 2017

The corresponding spec bug is whatwg/streams#653. @ricea has been meaning to work on that I believe, and we should indeed fix the reference implementation.

ricea
ricea previously requested changes Mar 30, 2017
View reviewed changes
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.

As the standard is currently written, implementations are not required to pass this test.

I'm planning to make implementations use the queue, but even after the change implementations will be able to take a "reasonable" amount of time to fill the queue, in order to provide freedom to optimise chunk size as implied by

  • Otherwise, WritableStreamDefaultWriterGetDesiredSize(writer) may be used to determine the flow rate heuristically, e.g. by delaying reads while it is judged to be "low" compared to the size of chunks that have been typically read.

If you are able to propose better standard language at whatwg/streams#653 that would be really great.

streams/piping/flow-control.js Outdated
const rs = recordingReadableStream({
pull(controller) {
controller.enqueue(unreadChunks.shift());
if (unreadChunks.length === 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Style nit: In stream code we always use { } with if, even single-line.

streams/piping/flow-control.js Outdated

const writer = ws.getWriter();
const firstWritePromise = writer.write('a');
assert_equals(writer.desiredSize, 2, 'after writing the writer\'s desiredSize must be 3');
Copy link
Contributor

Choose a reason for hiding this comment

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

"must be 2"

@domenic domenic mentioned this pull request Mar 30, 2017
Merged
@ghost
Copy link

ghost commented Mar 30, 2017

These tests are now available on w3c-test.org

@domenic
Copy link
Member

domenic commented Mar 30, 2017

I addressed the code review comments, so if @ricea is happy with this (and the spec change) we can merge them all together.

@ricea
Copy link
Contributor

ricea commented Mar 31, 2017

lgtm

@domenic domenic dismissed ricea’s stale review March 31, 2017 06:31

He said "lgtm" later

@domenic domenic merged commit 59b635d into web-platform-tests:master Mar 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ricea ricea ricea left review comments

Assignees
No one assigned
Labels
streams
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@isonmad @wpt-pr-bot @domenic @ricea