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 [[disturbed]] synchronously in ReadableStreamPipeTo #1022

Merged
merged 1 commit into from
Nov 13, 2019

Conversation

ricea
Copy link
Collaborator

@ricea ricea commented Nov 7, 2019

Set the [[disturbed]] internal slot to true in ReadableStreamPipeTo.
This is useful for the Fetch Standard. It is not visible to user code.

See whatwg/fetch#959 for the Fetch Standard
change that requires this.

Also fix two places where the name of the state internal slot was
mistakenly italicised.


Preview | Diff

Set the [[disturbed]] internal slot to true in ReadableStreamPipeTo.
This is useful for the Fetch Standard. It is not visible to user code.

See whatwg/fetch#959 for the Fetch Standard
change that requires this.
Copy link
Member

@yutakahirano yutakahirano left a comment

Choose a reason for hiding this comment

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

LGTM

@domenic
Copy link
Member

domenic commented Nov 7, 2019

It is not visible to user code.

Isn't it visible if someone does something like

response.body.pipeTo(...)
console.log(response.bodyUsed);

?

@ricea
Copy link
Collaborator Author

ricea commented Nov 8, 2019

Isn't it visible if someone does something like

response.body.pipeTo(...)
console.log(response.bodyUsed);

?

My original thought was that this depends on the implementation of pipeTo (ie. whether or not it does the first read synchronously). But I think you can make the difference detectable in any implementation by doing

response.body.pipeTo(new WritableStream({}, { highWaterMark: 0 });
console.log(response.bodyUsed);

So actually implementations will have to explicitly apply this change. Or maybe this standard is the wrong place to make this change, and it should go in Fetch instead.

@yutakahirano
Copy link
Member

Once locked and disturbed, there is no way to observe any internal state of a ReadableStream (until it gets unlocked). That is what I would expect for pipeTo, so I would like to put it in the spec.

@domenic
Copy link
Member

domenic commented Nov 8, 2019

Yeah, I am +1 on this change, I just think we may want to accompany it with some web platform tests.

@ricea
Copy link
Collaborator Author

ricea commented Nov 12, 2019

Please see the test at web-platform-tests/wpt#20205.

ricea added a commit to ricea/web-platform-tests that referenced this pull request Nov 13, 2019
whatwg/streams#1022 changes pipeTo and
pipeThrough to always set the [[disturbed]] slot of the Response's body
to true synchronously.

This test verifies that behaviour.
ricea added a commit to web-platform-tests/wpt that referenced this pull request Nov 13, 2019
whatwg/streams#1022 changes pipeTo and
pipeThrough to always set the [[disturbed]] slot of the Response's body
to true synchronously.

This test verifies that behaviour.
@ricea ricea merged commit e8fd846 into whatwg:master Nov 13, 2019
@ricea ricea deleted the piping-disturbs-synchronously branch November 13, 2019 01:05
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Nov 29, 2019
…sly, a=testonly

Automatic update from web-platform-tests
Verify piping disturbs stream synchronously (#20205)

whatwg/streams#1022 changes pipeTo and
pipeThrough to always set the [[disturbed]] slot of the Response's body
to true synchronously.

This test verifies that behaviour.
--

wpt-commits: 760485a4f652813d1f195ee28042c391cf2fdc02
wpt-pr: 20205
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Nov 29, 2019
…sly, a=testonly

Automatic update from web-platform-tests
Verify piping disturbs stream synchronously (#20205)

whatwg/streams#1022 changes pipeTo and
pipeThrough to always set the [[disturbed]] slot of the Response's body
to true synchronously.

This test verifies that behaviour.
--

wpt-commits: 760485a4f652813d1f195ee28042c391cf2fdc02
wpt-pr: 20205
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Nov 30, 2019
…sly, a=testonly

Automatic update from web-platform-tests
Verify piping disturbs stream synchronously (#20205)

whatwg/streams#1022 changes pipeTo and
pipeThrough to always set the [[disturbed]] slot of the Response's body
to true synchronously.

This test verifies that behaviour.
--

wpt-commits: 760485a4f652813d1f195ee28042c391cf2fdc02
wpt-pr: 20205

UltraBlame original commit: fcba8dc0552eb96c10b6997d7313972e5177ac93
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Nov 30, 2019
…sly, a=testonly

Automatic update from web-platform-tests
Verify piping disturbs stream synchronously (#20205)

whatwg/streams#1022 changes pipeTo and
pipeThrough to always set the [[disturbed]] slot of the Response's body
to true synchronously.

This test verifies that behaviour.
--

wpt-commits: 760485a4f652813d1f195ee28042c391cf2fdc02
wpt-pr: 20205

UltraBlame original commit: fcba8dc0552eb96c10b6997d7313972e5177ac93
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Nov 30, 2019
…sly, a=testonly

Automatic update from web-platform-tests
Verify piping disturbs stream synchronously (#20205)

whatwg/streams#1022 changes pipeTo and
pipeThrough to always set the [[disturbed]] slot of the Response's body
to true synchronously.

This test verifies that behaviour.
--

wpt-commits: 760485a4f652813d1f195ee28042c391cf2fdc02
wpt-pr: 20205

UltraBlame original commit: fcba8dc0552eb96c10b6997d7313972e5177ac93
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.

None yet

3 participants