Be clearer about how Fetch creates ReadableStreams. #781

jyasskin · 2018-08-01T21:54:10Z

It could be that this should call https://streams.spec.whatwg.org/#create-readable-stream or https://streams.spec.whatwg.org/#create-readable-byte-stream (#267?) instead of trying to go through the ReadableStream() constructor, with all the ambiguity about how modifying Object's prototype should affect things.

@domenic

Preview | Diff

ricea · 2018-08-02T02:23:28Z

Yes, I need to change this to use CreateReadableStream() but I haven't done it yet. Sorry.

jyasskin · 2018-08-03T21:58:14Z

@ricea Here's a terrible attempt at calling CreateReadableStream(). With some more work, it's probably possible to simplify that wording.

ricea · 2018-08-06T05:42:29Z

fetch.bs

@@ -1876,18 +1876,45 @@ with given <var>strategy</var>, <var>pull</var> action and <var>cancel</var> act
 are optional, run these steps:

 <ol>
- <li><p>Let <var>init</var> be a new object.
+ <li><p>Let <var>startAlg</var> be an algorithm that returns <code>undefined</code>.


Personally I find it clearer just to write out "startAlgorithm" in full.

ricea · 2018-08-06T05:50:57Z

fetch.bs

- <li><p>Let <var>stream</var> be the result of calling the initial value of {{ReadableStream}} as
- constructor with <var>init</var> and <var>strategy</var> if given.
+ <li><p>Let <var>highWaterMark</var> be:
+ <dl class=switch>


The need to unpack the strategy argument is unfortunate. If the reference in HTTP-Network-Fetch is the only usage, I would prefer to change that from the (scary) phrasing:

Let strategy be an object. The user agent may choose any object.

to something like

Let highWaterMark be a be a non-negative, non-NaN number, chosen by the user agent.
Let sizeAlgorithm be an algorithm that accepts chunk objects and returns a number, chosen by the user agent.

Yep. I didn't do that because I'm nervous about other callers outside Fetch, but I could leave a note here about the previous state and expect them to update.

I've done this and added the note.

ricea · 2018-08-06T05:56:31Z

fetch.bs

- <var>cancel</var> is given.
+ <dl class=switch>
+ <dt>If <var>pull</var> is given
+ <dd>the result of [=promise-calling=] <var>pull</var>().


@annevk usually objects to using the promises guide. However, I don't see much alternative except for copying the phrasing that the promises guide uses.

There's also https://streams.spec.whatwg.org/#promise-call, but it requires a non-undefined object parameter. @domenic, it looks like the underlying https://tc39.github.io/ecma262/#sec-call can take undefined there; would it make sense to allow that in PromiseCall()?

Very out-of-context question: What is the reasoning for objection to using the promises guide? Haven't read all of it, just curious (sorry this is not the best place).

@domfarolino Possibly w3ctag/promises-guide#52.

I know of one trap in there: because aggregating promises uses Promise.all() it calls the current (not original) value of Promise.reject() which may have been replaced. This makes it unsafe to use in algorithms that are not re-entrant or are otherwise vulnerable to unexpected JS execution.There may be other traps I don't know about.

Having said that, we use it in the streams standard and I think it is fine if you use it with care.

ricea · 2018-08-06T06:01:33Z

This is not so terrible. It seems pretty close to usable to me.

Some more bad news: the difference is actually observable if Object.prototype.start or Object.prototype.type is set. This is one motivation for moving to CreateReadableStream(). Someone needs to write a web platform test for this. I can do it if there are no other volunteers.

jyasskin

@ricea I'd appreciate if you'd write the web platform test. I'd probably need to do it in November after coming back from leave.

jyasskin · 2018-08-06T16:15:33Z

fetch.bs

@@ -1876,18 +1876,45 @@ with given <var>strategy</var>, <var>pull</var> action and <var>cancel</var> act
 are optional, run these steps:

 <ol>
- <li><p>Let <var>init</var> be a new object.
+ <li><p>Let <var>startAlg</var> be an algorithm that returns <code>undefined</code>.


jyasskin · 2018-08-06T16:38:33Z

fetch.bs

- <var>cancel</var> is given.
+ <dl class=switch>
+ <dt>If <var>pull</var> is given
+ <dd>the result of [=promise-calling=] <var>pull</var>().


There's also https://streams.spec.whatwg.org/#promise-call, but it requires a non-undefined object parameter. @domenic, it looks like the underlying https://tc39.github.io/ecma262/#sec-call can take undefined there; would it make sense to allow that in PromiseCall()?

jyasskin · 2018-08-06T17:11:15Z

fetch.bs

- <li><p>Let <var>stream</var> be the result of calling the initial value of {{ReadableStream}} as
- constructor with <var>init</var> and <var>strategy</var> if given.
+ <li><p>Let <var>highWaterMark</var> be:
+ <dl class=switch>


I've done this and added the note.

ricea · 2018-08-07T14:19:08Z

Okay, I will write the web platform test tomorrow.

This structurally looks good to me. I'm not sure whether we can leave a note in there. Last time I checked, the only spec relying on this was the service worker one. Maybe we can just file an issue there to update?

I think the markup will need some nitpicking, which I will leave to @annevk.

To match whatwg/fetch#781.

Test that setting Object.prototype.start to a function that throws doesn't interfere with fetch operations that create a stream. Similarly, verify that setting throwing accessors for 'type', 'size', and 'highWaterMark' doesn't cause problems. See whatwg/fetch#781 for background.

ricea · 2018-08-08T06:03:41Z

PR for the web platform test: web-platform-tests/wpt#12350

annevk · 2018-08-09T13:24:42Z

So https://w3c.github.io/ServiceWorker/ invokes this algorithm with strategy. I guess now it'd have to omit that argument, but still supply pull and cancel?

It seems rather muddy that this ends up using named arguments therefore. I think I'd rather we make them non-optional and require passing null. @domenic what do you think?

ricea · 2018-08-09T14:02:06Z

PR to update the ServiceWorker standard to match: w3c/ServiceWorker#1338.

domenic · 2018-08-09T16:06:24Z

Hmm yeah I didn't quite get the named arguments thing before. I dunno, this is OK-ish for now... Definitely a big improvement over the previous situation. I think the whole area of how streams interfaces with other specs needs a re-think, but I've been putting that off forever.

Also worth noting that, it seems like the idea of this wrapper, versus just using CreateReadableStream directly, is:

Omits startAlgorithm
Uses named arguments so that pullAlgorithm, and cancelAlgorithm become optional
Allows pullAlgorithm and cancelAlgorithm to return non-promises

annevk · 2018-08-09T16:18:54Z

Thanks @ricea, that looks good (modulo Oxford comma). Thoughts on my nit commit?

I guess I'm okay with merging this given @domenic's feedback.

jakearchibald · 2018-08-09T16:55:27Z

I'm making use of streams in background fetch, eg step 7 of https://wicg.github.io/background-fetch/#create-settled-fetches-algorithm, which is streaming an item from storage that may still be writing from another algorithm.

It felt like I wanted to kind of spec-level stream concept, but I haven't thought too much about it.

Anyway, the changes here don't disrupt background-fetch. The change is pretty easy.

ricea · 2018-08-10T06:46:09Z

The named arguments don't bother me. I think this is a clear improvement on the status quo.

As @domenic said, longer term we have to figure out a better way for other standards to talk about streams. I think this will tide us over in the meantime.

To match whatwg/fetch#781.

Update the strategy argument to 'construct a ReadableStream' according to whatwg/fetch#781.

Test that setting Object.prototype.start to a function that throws doesn't interfere with fetch operations that create a stream. Similarly, verify that setting throwing accessors for 'type', 'size', and 'highWaterMark' doesn't cause problems. See whatwg/fetch#781 for background.

…fere with fetch streams, a=testonly Automatic update from web-platform-testsFetch: Object.prototype shouldn't interfere with fetch streams Test that setting Object.prototype.start to a function that throws doesn't interfere with fetch operations that create a stream. Similarly, verify that setting throwing accessors for 'type', 'size', and 'highWaterMark' doesn't cause problems. See whatwg/fetch#781 for background. -- wpt-commits: ba582388f4506ea2c8341dac4d8ce6e5eb39871b wpt-pr: 12350

…fere with fetch streams, a=testonly Automatic update from web-platform-testsFetch: Object.prototype shouldn't interfere with fetch streams Test that setting Object.prototype.start to a function that throws doesn't interfere with fetch operations that create a stream. Similarly, verify that setting throwing accessors for 'type', 'size', and 'highWaterMark' doesn't cause problems. See whatwg/fetch#781 for background. -- wpt-commits: ba582388f4506ea2c8341dac4d8ce6e5eb39871b wpt-pr: 12350 UltraBlame original commit: a2a4a8d57735d173482c452d09ae4eb57ecfe1fe

…fere with fetch streams, a=testonly Automatic update from web-platform-testsFetch: Object.prototype shouldn't interfere with fetch streams Test that setting Object.prototype.start to a function that throws doesn't interfere with fetch operations that create a stream. Similarly, verify that setting throwing accessors for 'type', 'size', and 'highWaterMark' doesn't cause problems. See whatwg/fetch#781 for background. -- wpt-commits: ba582388f4506ea2c8341dac4d8ce6e5eb39871b wpt-pr: 12350 UltraBlame original commit: d03965cb9d48c88a94030dc00aa15d01b50f63a8

…fere with fetch streams, a=testonly Automatic update from web-platform-testsFetch: Object.prototype shouldn't interfere with fetch streams Test that setting Object.prototype.start to a function that throws doesn't interfere with fetch operations that create a stream. Similarly, verify that setting throwing accessors for 'type', 'size', and 'highWaterMark' doesn't cause problems. See whatwg/fetch#781 for background. -- wpt-commits: ba582388f4506ea2c8341dac4d8ce6e5eb39871b wpt-pr: 12350 UltraBlame original commit: a2a4a8d57735d173482c452d09ae4eb57ecfe1fe

…fere with fetch streams, a=testonly Automatic update from web-platform-testsFetch: Object.prototype shouldn't interfere with fetch streams Test that setting Object.prototype.start to a function that throws doesn't interfere with fetch operations that create a stream. Similarly, verify that setting throwing accessors for 'type', 'size', and 'highWaterMark' doesn't cause problems. See whatwg/fetch#781 for background. -- wpt-commits: ba582388f4506ea2c8341dac4d8ce6e5eb39871b wpt-pr: 12350 UltraBlame original commit: d03965cb9d48c88a94030dc00aa15d01b50f63a8

…fere with fetch streams, a=testonly Automatic update from web-platform-testsFetch: Object.prototype shouldn't interfere with fetch streams Test that setting Object.prototype.start to a function that throws doesn't interfere with fetch operations that create a stream. Similarly, verify that setting throwing accessors for 'type', 'size', and 'highWaterMark' doesn't cause problems. See whatwg/fetch#781 for background. -- wpt-commits: ba582388f4506ea2c8341dac4d8ce6e5eb39871b wpt-pr: 12350 UltraBlame original commit: a2a4a8d57735d173482c452d09ae4eb57ecfe1fe

…fere with fetch streams, a=testonly Automatic update from web-platform-testsFetch: Object.prototype shouldn't interfere with fetch streams Test that setting Object.prototype.start to a function that throws doesn't interfere with fetch operations that create a stream. Similarly, verify that setting throwing accessors for 'type', 'size', and 'highWaterMark' doesn't cause problems. See whatwg/fetch#781 for background. -- wpt-commits: ba582388f4506ea2c8341dac4d8ce6e5eb39871b wpt-pr: 12350 UltraBlame original commit: d03965cb9d48c88a94030dc00aa15d01b50f63a8

jyasskin force-pushed the ReadableStream-construction branch from 46e1be1 to 3f3479e Compare August 3, 2018 21:57

jyasskin added 2 commits August 3, 2018 14:57

Be clearer about the underlying source of a constructed ReadableStream.

c7893b6

Call CreateReadableStream instead of the ReadableStream() constructor.

a3e0245

jyasskin force-pushed the ReadableStream-construction branch from 3f3479e to a3e0245 Compare August 3, 2018 21:57

ricea reviewed Aug 6, 2018

View reviewed changes

Replace strategy param with its contents.

b80938e

jyasskin commented Aug 6, 2018

View reviewed changes

jyasskin added a commit to jyasskin/ServiceWorker that referenced this pull request Aug 7, 2018

Update the strategy argument to 'construct a ReadableStream'.

77dd83f

To match whatwg/fetch#781.

jyasskin mentioned this pull request Aug 7, 2018

Update the strategy argument to 'construct a ReadableStream'. w3c/ServiceWorker#1338

Merged

ricea mentioned this pull request Aug 8, 2018

[Fetch] Object.prototype shouldn't interfere with fetch streams web-platform-tests/wpt#12350

Merged

jyasskin changed the title ~~Be clearer about the underlying source of a constructed ReadableStream.~~ Be clearer about Fetch creates ReadableStreams. Aug 8, 2018

jyasskin changed the title ~~Be clearer about Fetch creates ReadableStreams.~~ Be clearer about how Fetch creates ReadableStreams. Aug 8, 2018

initial nits

f514b21

jakearchibald mentioned this pull request Aug 9, 2018

Update stream usage WICG/background-fetch#104

Closed

annevk merged commit 6b31c45 into whatwg:master Aug 10, 2018

jyasskin added a commit to jyasskin/ServiceWorker that referenced this pull request Aug 13, 2018

Update the strategy argument to 'construct a ReadableStream'.

168d0b9

To match whatwg/fetch#781.

jyasskin deleted the ReadableStream-construction branch August 13, 2018 02:52

jungkees pushed a commit to w3c/ServiceWorker that referenced this pull request Aug 13, 2018

Update the strategy argument to 'construct a ReadableStream'. (#1338)

a71caad

Update the strategy argument to 'construct a ReadableStream' according to whatwg/fetch#781.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Be clearer about how Fetch creates ReadableStreams. #781

Be clearer about how Fetch creates ReadableStreams. #781

jyasskin commented Aug 1, 2018 •

edited by pr-preview bot

Loading

ricea commented Aug 2, 2018

jyasskin commented Aug 3, 2018

ricea Aug 6, 2018

jyasskin Aug 6, 2018

ricea Aug 6, 2018

jyasskin Aug 6, 2018

jyasskin Aug 6, 2018

ricea Aug 6, 2018

jyasskin Aug 6, 2018

domfarolino Aug 8, 2018

ricea Aug 9, 2018

ricea commented Aug 6, 2018

jyasskin left a comment

jyasskin Aug 6, 2018

jyasskin Aug 6, 2018

jyasskin Aug 6, 2018

ricea commented Aug 7, 2018

ricea commented Aug 8, 2018

annevk commented Aug 9, 2018

ricea commented Aug 9, 2018

domenic commented Aug 9, 2018

annevk commented Aug 9, 2018

jakearchibald commented Aug 9, 2018

ricea commented Aug 10, 2018

Be clearer about how Fetch creates ReadableStreams. #781

Be clearer about how Fetch creates ReadableStreams. #781

Conversation

jyasskin commented Aug 1, 2018 • edited by pr-preview bot Loading

ricea commented Aug 2, 2018

jyasskin commented Aug 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricea commented Aug 6, 2018

jyasskin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricea commented Aug 7, 2018

ricea commented Aug 8, 2018

annevk commented Aug 9, 2018

ricea commented Aug 9, 2018

domenic commented Aug 9, 2018

annevk commented Aug 9, 2018

jakearchibald commented Aug 9, 2018

ricea commented Aug 10, 2018

jyasskin commented Aug 1, 2018 •

edited by pr-preview bot

Loading