Requirement on promises equality #266

tyoshino · 2015-01-19T13:16:57Z

In the spec, we remember and return the same promises for efficiency if it's semantically correct. The algorithms we have in the spec are representing this. This may look like a requirement to the spec readers. And, we have some unit test items where we actually check this kind of equality.

Do you think this really should be a requirement?

I'm asking this since:

When implementing the Streams in Blink, we encountered some difficulties with ensuring some of the equality we can infer from the algorithm of the methods/getters.
There seems to be no real practical needs for such a requirement.

domenic · 2015-01-19T20:03:40Z

I think it should definitely be a requirement. It's very strange to have JavaScript properties changing all the time such that e.g. rs.ready !== rs.ready. They should only change when they need to.

tyoshino · 2015-01-20T03:25:11Z

@domenic OK. So, it means that rs.ready should return the same instance until we're asked to replace [[readyPromise]] on e.g. "readable" to "waiting" transition?

How about the result of cancel() call on a "closed" or "errored" stream? It's now specified to return [[closedPromise]] than creating a new one resolved/rejected for efficiency. Do you think it also should be a requirement?

BTW, writableStream.close() can also be simplified to return its [[closedPromise]] when the stream is in the "closed" or "errored" state.

/cc @yutakahirano

domenic · 2015-01-20T04:48:20Z

So, it means that rs.ready should return the same instance until we're asked to replace [[readyPromise]] on e.g. "readable" to "waiting" transition?

Yeah, definitely.

How about the result of cancel() call on a "closed" or "errored" stream? It's now specified to return [[closedPromise]] than creating a new one resolved/rejected for efficiency. Do you think it also should be a requirement?

We should make sure to normatively specify and test one way or another. But since it's a method instead of property I would be OK either way. I think the way it is now makes some sense but I'd be OK either way. We just need to make sure all implementations agree.

domenic · 2015-02-02T20:54:54Z

@tyoshino @yhirano as implementers, any thoughts one way or the other, for cancel(), close(), abort(), or any other methods? I am OK specifying them as new promises in all cases if that seems desirable. It's a bit nicer if the methods don't share return values with other places in the system, but it is a minor efficiency loss, so I could go either way.

yutakahirano · 2015-02-03T04:37:44Z

I'm concerned about the spec complexity. .ready and .closed changed in recent two weeks. Currently they are defined very cleanly, but can we keep the readability when we start caching?

I don't see any reason to keep promise identity on "cancel()" return value.

FYI, as @tyoshino said, we have some difficulties to ensure the identity. Currently I'm not sure if it is worth ensuring.

yutakahirano · 2015-02-03T04:50:37Z

var stream = ...;
assert(stream.state === 'waiting');
var p = stream.ready;
var reader = stream.getReader();
var q = stream.ready;
reader.releaseLock();
var r = stream.ready;

Can we ensure p === q === r? Then, it might be helpful to solve yutakahirano/fetch-with-streams#15 (comment).

domenic · 2015-02-03T06:11:37Z

I'm concerned about the spec complexity. .ready and .closed changed in recent two weeks.

I apologize for the churn :(. The reader stuff certainly shook things up. Hopefully should be pretty stable now...

Currently they are defined very cleanly, but can we keep the readability when we start caching?

I don't quite understand. The definition should not change due to caching. (In fact I don't think of "caching" as a separate thing; they are just getters for values which sometimes change and sometimes don't. Similar to state.)

FYI, as @tyoshino said, we have some difficulties to ensure the identity. Currently I'm not sure if it is worth ensuring.

Well, they have to have identity for the getters, otherwise stream.ready !== stream.ready. My question was more about the methods.

Can we ensure p === q === r?

I don't think so, unless we significantly change the ergonomics of how readers work. Since rs.ready must never fulfill for a locked stream and reader.ready must definitely fulfill for a reader for a locked stream that becomes readable, I don't see how to make these compatible.

tyoshino · 2015-02-03T14:25:26Z

Can we ensure p === q === r?

I don't think so, unless we significantly change the ergonomics of how readers work. Since rs.ready must

I think Yutaka meant whether we can make p === q === r when the stream stays to be waiting or not.

Of course, we cannot return the same promise instance beyond "readable" to "waiting" transition. We discussed how much we can keep it unchanged.

The ideally stable behavior is that as far as there's no state transition visible to stream user (not reader user) happened, rs.ready returns the same promise. We can realize this by caching the promise returned at p and keep returning it until "readable" to "waiting" state transition happens and the transition becomes visible to the user (the stream is unlocked). But to implement this, we need to change the fulfillment callback to call some internal only method since rs.ready would then return the cached promise.

We haven't given it a try, but maybe it'll be a little more complicated. We'd like to discuss how much stability is required.

domenic · 2015-02-03T21:05:42Z

I think I see. The problem is that the current spec actually violates rs.ready === rs.ready in many cases---somehow I missed that this would happen. It would indeed be nice to fix that. I will look in to it.

domenic · 2015-02-03T22:05:26Z

BTW I now understand why there was so much confusion. I was saying one thing---that the getter should return stable values---and then the spec was saying another thing. I didn't mean to imply that it was the implementation's job to keep the values stable even if the spec can't figure it out! I will fix the spec :)

domenic · 2015-02-04T00:12:37Z

I would like you guys to take a look at my attempt at fixing this for .ready. If it looks good I will do so for ExclusiveStreamReader as well, and update the spec.

(An interesting point to note is that I tested by commenting out each "cache invalidation" and every one causes at least some tests to fail or hang. So that's good!)

I experimented for a couple hours with approaches that didn't add another internal slot. For example, moving the .then(() => this.ready) logic to resolution-time. But eventually I realized that was impossible, because readers need to be able to update the "true" promise while the public API cannot return that, so there needs to be at least two.

Although this is more code, I think that this approach is strictly more efficient (in that strictly fewer objects are created). Even if we moved everything to methods that return fresh objects every time, that would be worse for authors with regard to efficiency, in addition to being more awkward.

yutakahirano · 2015-02-04T04:04:22Z

If we ensure .ready object identity and specify that breaks when the internal state changes, a user can observe an internal state change even when locked. That is not desirable.

var stream = ...;
assert stream is locked to someone;

var p = stream.ready;
...
var q = stream.ready;
if (p !== q) {
  // I cannot see the internal state, but I'm sure something happens!
}

tyoshino · 2015-02-04T15:44:36Z

Domenic. Had a look at your change. It's nice work but as Yutaka pointed out, it's revealing the internal state.

This issue made me rethink the whole architecture. We gave up a lot recently such as that the reader can be used for stream subclasses, etc. It looks the current implementation is twisted for no longer valid requirements.

I made a PR #277 which uses a lot of interaction on hidden variables between the stream and reader but realizes the promise identity requirement perfectly (i.e. the ideal stability in #266 (comment)). Do you think this violates any of still-valid design requirements?

By having each of the stream and the reader hold promises, and more directly reach into each other, we can get better behavior, e.g. avoiding `rs.ready !== rs.ready` and other problems. See discussion in #266.

tyoshino · 2015-02-10T03:35:22Z

#277 is closed. Now the spec and reference impl ensures the requirement #266 (comment).

Now, the question is if we require all the implementor to follow the requirement in the spec.

domenic · 2015-02-10T05:53:33Z

Well, yes, we definitely do; that's why there's a spec.

The other question is what we want to do for methods. We could make them return fresh promises each time.

Addresses #266 ReadableByteStream version of 9ec484b

tyoshino · 2015-02-10T15:18:33Z

The other question is what we want to do for methods. We could make them return fresh promises each time.

cancel(reason) being called during "closed" or "errored" is just a mistake. I don't care whether it would return something stable or not.

Same about ws.abort(reason) on "closed" or "errored" WritableStream, and ws.close() on an "errored" WritableStream, and ws.write() on an "errored" WritableStream.

IIRC, I've been thinking that they should just return a promise rejected with a TypeError indicating invalid state error. Sorry if I'm missing something discussed in the past.

domenic · 2015-02-10T17:09:17Z

IIRC, I've been thinking that they should just return a promise rejected with a TypeError indicating invalid state error. Sorry if I'm missing something discussed in the past.

I'm sure they were but I kind of forget the reasoning too :). It was very early days.

I think the idea was that your intent is kind of to shut down the stream and make sure no resources leak. And if you're already closed/errored then you've already accomplished that so you shouldn't get an error. But, I see the perspective of wanting to tell developers they are being overzealous too.

I will see if we can get any opinions from Twitter (that hasn't been too successful in the past) and we could also survey other libraries (both for streams and for other disposable resources). But I am OK with making them TypeErrors.

calvaris · 2015-02-10T18:18:00Z

For me it makes sense a TypeError in those cases.

Addresses #266 ReadableByteStream version of 9ec484b

domenic · 2015-02-10T19:35:29Z

We have two votes in IRC in favor of no-op.

wanderview says:

I prefer silent... you're asking it to enter a state... its already in that state... not an error (in my opinion)

JakeA says:

agree with wanderview. It's in the spirit of clearTimeout(123456)

On Twitter though we have indutny with

fail out loudly.

and bradleymeck with

silent on the result (don't make a closed stream turn into error), error on the caller pretty violently.

(I'm not @-mentioning anyone since I don't feel a need for them to get emails for everything else on this thread.)

So, uh, I dunno.

jameshartig · 2015-02-10T20:57:41Z

If you're silent then the dev has no way of knowing if it didn't do anything (since it's already closed) but if you throw an error they can at least catch that and ignore it if they're okay being overzealous.

wanderview · 2015-02-10T20:59:12Z

I prefer not-an-error, but I do agree having a return value that means "I did something" would be nice.

This replaces the dual ready + read() approach previously, which was derived from the epoll(7) + read(2) paradigm. In #253 we did a deep dive on real-world implementation strategies for byte streams, and discovered that the ready + read() model causes a conflict with the semantics we want for byte streams. Briefly, because some byte streams will demand to know the size of the buffer they must fill before doing any I/O (the fread(3) model), the byte stream method that reads into a given typed array must be asynchronous. If such byte streams are then to conform to the readable stream interface, with a no-argument read() method that is an auto-allocating version of the read-into method, then read() must also be async, across all readable streams. This is a usability upgrade for consumers, in many cases. However, for non-byte streams, it potentially costs more microtasks when multiple chunks of data would be available synchronously. We are hopeful that even if current JS engine microtask queues are not as fast as they could be, they will improve over time until this overhead is negligible (which in theory it should be). Alongside this change, we moved the read() method from the readable stream to the reader (now called "readable stream reader" instead of "exclusive stream reader"). This drastically simplifies the stream/reader interaction, and also allows the possibility of different types of readers which have different reading behavior---again, very helpful for byte streams. The usability impact is also positive, as it guides consumers toward using piping instead of directly reading chunks from the stream. Note that read() promises fulfill with { value, done } instead of using an EOS sentinel value. This avoids a number of problems (see extensive discussion in #253), and also provides a mechanism by which readable byte streams can smuggle out "unused" buffers given to them, by using { value: zeroLengthViewOntoBuffer, done: true }. Finally, the state property is now removed (from readable stream), since there is no longer a "waiting" vs. "readable" distinction. This commit also adds some new infrastructure for _templated tests_, and ports some portion of the existing tests there. This is our solution for #217 and #264. Note that we re-merged all related code into a single readable-stream.js file, as the setup with the three separate files (readable-stream.js, exclusive-stream-reader.js, and readable-stream-abstract-ops.js) was problematic in causing circular dependencies. - Closes #253! - Closes #266 by simplifying reader-related stuff, removing the problematic `ready` promise, and ensuring that cancel() always returns a new promise instead of reusing [[closedPromise]]. - Closes #291 since the relevant tests have been re-written and the related infrastructure around pull simplified. - Closes #264 by introducing templated tests.

This replaces the dual ready + read() approach previously, which was derived from the epoll(7) + read(2) paradigm. In #253 we did a deep dive on real-world implementation strategies for byte streams, and discovered that the ready + read() model causes a conflict with the semantics we want for byte streams. Briefly, because some byte streams will demand to know the size of the buffer they must fill before doing any I/O (the fread(3) model), the byte stream method that reads into a given typed array must be asynchronous. If such byte streams are then to conform to the readable stream interface, with a no-argument read() method that is an auto-allocating version of the read-into method, then read() must also be async, across all readable streams. This is a usability upgrade for consumers, in many cases. However, for non-byte streams, it potentially costs more microtasks when multiple chunks of data would be available synchronously. We are hopeful that even if current JS engine microtask queues are not as fast as they could be, they will improve over time until this overhead is negligible (which in theory it should be). Alongside this change, we moved the read() method from the readable stream to the reader (now called "readable stream reader" instead of "exclusive stream reader"). This drastically simplifies the stream/reader interaction, and also allows the possibility of different types of readers which have different reading behavior---again, very helpful for byte streams. The usability impact is also positive, as it guides consumers toward using piping instead of directly reading chunks from the stream. Note that read() promises fulfill with { value, done } instead of using an EOS sentinel value. This avoids a number of problems (see extensive discussion in #253), and also provides a mechanism by which readable byte streams can smuggle out "unused" buffers given to them, by using { value: zeroLengthViewOntoBuffer, done: true }. Finally, the state property is now removed (from readable stream), since there is no longer a "waiting" vs. "readable" distinction. This commit also adds some new infrastructure for _templated tests_, and ports some portion of the existing tests there. This is our solution for #217 and #264. Note that we re-merged all related code into a single readable-stream.js file, as the setup with the three separate files (readable-stream.js, exclusive-stream-reader.js, and readable-stream-abstract-ops.js) was problematic in causing circular dependencies. - Closes #253! - Closes #266 by simplifying reader-related stuff, removing the problematic `ready` promise, and ensuring that cancel() always returns a new promise instead of reusing [[closedPromise]]. - Closes #264 by introducing templated tests.

tyoshino · 2015-03-17T09:19:32Z

Re: equality,

See this comment #296 (comment) of PR #296.

If we decide to put .closed back to the stream (not reader) in the future, we need to remember this.

Re: cancel() on errored/closed stream (#266 (comment)),

returns a resolved promise when in closed state
returns a rejected promise when in errored state

As we no longer have any method to probe the current state of the stream synchronously, we should just expect cancel() being called when in closed or errored state. I think the current implementation is reasonable for the new async read() style readable stream/reader.

#266 (comment)

domenic added question implementer concern labels Jan 20, 2015

domenic added this to the Week of 2015-02-02 milestone Feb 2, 2015

domenic assigned tyoshino Feb 4, 2015

tyoshino mentioned this issue Feb 10, 2015

Add readableAmount getter to ReadableByteStream #279

Closed

tyoshino added a commit that referenced this issue Feb 10, 2015

Simplify stream/reader promises properties on ReadableByteStream

8550485

Addresses #266 ReadableByteStream version of 9ec484b

tyoshino added a commit that referenced this issue Feb 10, 2015

Simplify stream/reader promises properties on ReadableByteStream

2011868

Addresses #266 ReadableByteStream version of 9ec484b

domenic removed this from the Week of 2015-02-02 milestone Mar 3, 2015

domenic mentioned this issue Mar 12, 2015

Change the model for ReadableStream to have async read() #296

Merged

domenic closed this as completed in #296 Mar 17, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Requirement on promises equality #266

Requirement on promises equality #266

tyoshino commented Jan 19, 2015

domenic commented Jan 19, 2015

tyoshino commented Jan 20, 2015

domenic commented Jan 20, 2015

domenic commented Feb 2, 2015

yutakahirano commented Feb 3, 2015

yutakahirano commented Feb 3, 2015

domenic commented Feb 3, 2015

tyoshino commented Feb 3, 2015

domenic commented Feb 3, 2015

domenic commented Feb 3, 2015

domenic commented Feb 4, 2015

yutakahirano commented Feb 4, 2015

tyoshino commented Feb 4, 2015

tyoshino commented Feb 10, 2015

domenic commented Feb 10, 2015

tyoshino commented Feb 10, 2015

domenic commented Feb 10, 2015

calvaris commented Feb 10, 2015

domenic commented Feb 10, 2015

jameshartig commented Feb 10, 2015

wanderview commented Feb 10, 2015

tyoshino commented Mar 17, 2015

Requirement on promises equality #266

Requirement on promises equality #266

Comments

tyoshino commented Jan 19, 2015

domenic commented Jan 19, 2015

tyoshino commented Jan 20, 2015

domenic commented Jan 20, 2015

domenic commented Feb 2, 2015

yutakahirano commented Feb 3, 2015

yutakahirano commented Feb 3, 2015

domenic commented Feb 3, 2015

tyoshino commented Feb 3, 2015

domenic commented Feb 3, 2015

domenic commented Feb 3, 2015

domenic commented Feb 4, 2015

yutakahirano commented Feb 4, 2015

tyoshino commented Feb 4, 2015

tyoshino commented Feb 10, 2015

domenic commented Feb 10, 2015

tyoshino commented Feb 10, 2015

domenic commented Feb 10, 2015

calvaris commented Feb 10, 2015

domenic commented Feb 10, 2015

jameshartig commented Feb 10, 2015

wanderview commented Feb 10, 2015

tyoshino commented Mar 17, 2015