Add IsReadableStreamDisturbed predicate #385

tyoshino · 2015-08-03T09:32:44Z

See the suggestion in the OP of #378.

tyoshino · 2015-08-03T11:22:58Z

The disturbed flag is never changed when the streams is closed. Is it assumed that this is implemented after removing the auto-release feature?

yutakahirano · 2015-08-03T12:31:31Z

FYI: yutakahirano/fetch-with-streams#48 (comment)

domenic · 2015-08-03T15:58:41Z

index.bs

@@ -300,6 +300,11 @@ Instances of <code>ReadableStream</code> are created with the internal slots des
      state and queue of this stream
  </tr>
  <tr>
+    <td>\[[disturbed]]
+    <td>A boolean flag set to <b>true</b> when either <code>read()</code> or <code>cancel()</code> has been called on


better phrased as "when the stream has been read from or canceled"

domenic · 2015-08-03T16:02:47Z

The disturbed flag is never changed when the streams is closed. Is it assumed that this is implemented after removing the auto-release feature?

I don't think we need to remove auto-release. However, as @yutakahirano notes, I think it was important (for reasons I forget) to have the disturbed flag change even on closed streams. As such it should probably move up a few steps in the algorithms.

tyoshino · 2015-08-04T04:36:15Z

Maybe one of the origins of the requirement is yutakahirano/fetch-with-streams#37 (comment) ? Is it still active? whatwg/fetch#61 (comment) Or even if it's active, it'll be covered by used flag?

domenic · 2015-08-04T16:05:24Z

Reviewing https://etherpad.mozilla.org/streams-f2f-july "Even more conservative strawman" I think the requirement comes from wanting .json() etc. to automatically make the stream disturbed, even for empty streams. The used flag is only for new Request(otherRequest). This is important because it means that author code that acts similar to json() will also make the stream disturbed.

tyoshino · 2015-08-05T09:10:35Z

Addressed the comment. Thanks.

This is important because it means that author code that acts similar to json() will also make the stream disturbed.

OK. But an operation made on a released reader making the ex-associated stream disturbed sounds strange.

domenic · 2015-08-05T21:10:32Z

But an operation made on a released reader making the ex-associated stream disturbed sounds strange.

Oh, you are right. It would mean that getting a reader for a closed/error stream would still have to hold on to the stream in order to maybe mark it disturbed.

This seems a bit silly. It means you cannot GC the stream if the reader is still alive, even though the stream is closed or errored.

But that is maybe not so bad since closed or errored streams do not have any data in their internal queue.

Also it would eliminate some internal complexity from the reader (the [[state]] and [[storedError]] fields).

And the silliness seems less bad than debating bodyUsed all over again.

What do you think?

domenic · 2015-08-17T23:31:24Z

Ping. What are your thoughts?

yutakahirano · 2015-08-18T03:59:49Z

I prefer dropping auto-release to introducing released-but-still-connected state.

domenic · 2015-08-18T16:48:39Z

That seems like a good compromise to me. @tyoshino do you want to do the PR or shall I?

domenic · 2015-08-18T17:30:06Z

Actually, I can do that; there is other work I want to do on top of it. I will send a PR later today.

Per #385, the current auto-unlocking design interferes with the implementation of the disturbed flag for fetch. It was never that useful anyway, so removing it is probably the best solution.

domenic · 2015-08-18T21:43:09Z

OK, I have pushed to this PR with a base commit that removes automatic unlocking on close/error.

Two important questions:

Should we now reject read() and closed when used on released readers, instead of treating as a closed/errored stream? That seems simpler. I have not implemented it yet.
Are we OK with breaking compat on this? It would be a breaking change to Chrome's current behavior, and a change in an area of the spec we've called stable. I personally think it is unlikely to break things, but what do you think? /cc @yutakahirano @calvaris

calvaris · 2015-08-19T07:32:13Z

I think it is also unlikely that we break things.

Our current implementation is also based on closed/errored so it would mean to change our current implementation, which I'm not a big fan of but it wouldn't be a big change either, so I'm against but only barely and based on laziness :)

What I would do is securing the change of behavior if it happens with some tests ensuring that everything is as it should be and not as it was.

domenic · 2015-08-19T14:57:47Z

@calvaris thanks for weighing in! I made a bunch of test updates in 9945ae1 but if you can think of others we should add them. E.g. if you come up with test descriptions I'll write the tests :)

yutakahirano · 2015-08-19T15:02:54Z

I believe we cannot introduce IsDisturbed with satisfying all proposed requirements and all existing properties. I think dropping the auto-release is a tolerable way.

domenic · 2015-08-19T15:05:16Z

@yutakahirano well, we can introduce it without any backward-incompatible changes if we follow #385 (comment) and allow readers to retain references to their owner stream even after release.

But I take it you think that is bad?

yutakahirano · 2015-08-19T15:08:56Z

But I take it you think that is bad?

Yes.

domenic · 2015-08-19T16:26:08Z

OK. Does anyone have any opinion on

Should we now reject read() and closed when used on released readers, instead of treating as a closed/errored stream?

domenic · 2015-08-20T20:14:02Z

My opinion is that they should now reject. That will allow us to get rid of the internal slots [[state]] and [[storedError]]. The downside is that you would no longer get to treat released readers as closed/errored streams depending on their originating stream. Instead they would effectively behave as if they always came from errored streams. But I don't think that's a big deal; you shouldn't be messing with the reader after it's been released.

tyoshino · 2015-08-24T09:03:53Z

Sorry for delay. I'm fine with the plan. I don't come up with any reasonable use case to keep a reference to a reader while releasing the parent stream. Some code may be accidentally doing so, but could be easily fixed and as Domenic said it's not so harmful to hold an errored/closed stream (which is small) in addition to a reader.

I'm also fine with making read on released reader reject. I think currently people who are unaware of the auto release mechanism are not calling release(), and basically people who are aware of wouldn't read() on released one. Some may be using release() and also using released reader as a representation of closed stream that accepts read() and tell us that the stream is closed repeatedly. But ... maybe the number of users with such a use case is small, I guess.

domenic · 2015-08-25T00:54:07Z

OK. Updated with what I think should be final. Cannot build the spec until speced/bikeshed#466 is fixed, but ready for review.

tyoshino · 2015-08-26T08:02:24Z

index.bs

@@ -855,7 +848,10 @@ Instances of <code>ReadableStreamReader</code> are created with the internal slo
  1. If IsReadableStreamReader(*this*) is *false*, throw a *TypeError* exception.
  1. If *this*@[[ownerReadableStream]] is *undefined*, return *undefined*.
  1. If *this*@[[readRequests]] is not empty, throw a *TypeError* exception.
-  1. Perform CloseReadableStreamReader(*this*).
+  1. If *this*@[[ownerReadableStream]]@[[state]] is `"readable"`, reject *this*@[[closedPromise]] with a *TypeError* exception.
+  1. Otherwise, let *this*@[[closedPromise]] be a new promise rejected with a *TypeError* exception.


I thought "let" is used for declaring/substituting to local variables while set is used for substituting new value to a slot. I'm just misunderstanding your style rule or this "let" should be "set"?

Do we really need to set a new rejected promise to [[closedPromise]] here?

Ah, I see. This new rejected promise and the TypeError at L851 are representing "already released". Then, looks good.

tyoshino · 2015-08-26T12:23:25Z

Review done!

Per #385, the current auto-unlocking design interferes with the implementation of the disturbed flag for fetch. It was never that useful anyway, so removing it is probably the best solution.

Closes #378.

This significantly simplifies the internals of readers, and their interaction with their owner stream.

domenic · 2015-08-27T19:53:16Z

Thanks for the review! I turns out I failed to do spec changes for the "Remove automatic unlocking on close/error" commit. Your let vs. set catch was correct as well. Fixed and will merge now.

yutakahirano · 2015-09-07T07:14:10Z

Just a confirmation: @wanderview, are you OK with this change? I am planning to implement this behavior change to Chrome, and before that I would like to make sure that everyone is satisfied.

yutakahirano · 2015-09-11T02:18:07Z

@wanderview: ping

wanderview · 2015-09-11T02:26:15Z

I'm sorry, but I haven't been able to keep up with this. Can you summarize
the differences from the July f2f agreement?

Are we just dropping automatic unlocking on stream close?
On Sep 10, 2015 10:18 PM, "Yutaka Hirano" notifications@github.com wrote:

@wanderview https://github.com/wanderview: ping

—
Reply to this email directly or view it on GitHub
#385 (comment).

yutakahirano · 2015-09-11T04:01:15Z

The f2f agreement suggests a stream should be disturbed when read even if it is closed (See yutakahirano/fetch-with-streams#48 (comment)). In order to achieve that, there are two ways:

Auto-released reader makes the stream disturbed.
Remove the auto-release mechanism.

My opinion is that the latter is less evil.

Are we just dropping automatic unlocking on stream close?

And on stream error.

wanderview · 2015-09-11T13:35:31Z

Remove the auto-release mechanism.

I think this is reasonable. Thanks for summarizing for me.

tyoshino mentioned this pull request Aug 3, 2015

Add IsDisturbed predicate #378

Closed

domenic reviewed Aug 3, 2015
View reviewed changes

domenic force-pushed the isdisturbed branch from e621ecf to 6778650 Compare August 18, 2015 21:38

domenic force-pushed the isdisturbed branch from 6778650 to 8a4c61c Compare August 18, 2015 21:39

domenic added this to the Week of 2015-08-24 milestone Aug 20, 2015

domenic force-pushed the isdisturbed branch from 0f2ce04 to 341fa5e Compare August 25, 2015 00:53

tyoshino reviewed Aug 26, 2015
View reviewed changes

domenic and others added 4 commits August 27, 2015 15:49

Remove automatic unlocking on close/error

ddcf083

Per #385, the current auto-unlocking design interferes with the implementation of the disturbed flag for fetch. It was never that useful anyway, so removing it is probably the best solution.

Add IsReadableStreamDisturbed predicate

6f4bb19

Closes #378.

Update IsReadableStreamDisturbed to work after close or error

bd839e5

Make reader closed and read() reject after release

632b26a

This significantly simplifies the internals of readers, and their interaction with their owner stream.

domenic force-pushed the isdisturbed branch from 341fa5e to 632b26a Compare August 27, 2015 19:52

domenic merged commit 632b26a into master Aug 27, 2015

tyoshino deleted the isdisturbed branch September 14, 2015 09:31

tyoshino mentioned this pull request Sep 14, 2015

problem with .text() drain behavior and .bodyUsed yutakahirano/fetch-with-streams#37

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add IsReadableStreamDisturbed predicate #385

Add IsReadableStreamDisturbed predicate #385

tyoshino commented Aug 3, 2015

tyoshino commented Aug 3, 2015

yutakahirano commented Aug 3, 2015

domenic Aug 3, 2015

tyoshino Aug 4, 2015

domenic commented Aug 3, 2015

tyoshino commented Aug 4, 2015

domenic commented Aug 4, 2015

tyoshino commented Aug 5, 2015

domenic commented Aug 5, 2015

domenic commented Aug 17, 2015

yutakahirano commented Aug 18, 2015

domenic commented Aug 18, 2015

domenic commented Aug 18, 2015

domenic commented Aug 18, 2015

calvaris commented Aug 19, 2015

domenic commented Aug 19, 2015

yutakahirano commented Aug 19, 2015

domenic commented Aug 19, 2015

yutakahirano commented Aug 19, 2015

domenic commented Aug 19, 2015

domenic commented Aug 20, 2015

tyoshino commented Aug 24, 2015

domenic commented Aug 25, 2015

tyoshino Aug 26, 2015

tyoshino Aug 26, 2015

tyoshino Aug 26, 2015

tyoshino commented Aug 26, 2015

domenic commented Aug 27, 2015

yutakahirano commented Sep 7, 2015

yutakahirano commented Sep 11, 2015

wanderview commented Sep 11, 2015

yutakahirano commented Sep 11, 2015

wanderview commented Sep 11, 2015

Add IsReadableStreamDisturbed predicate #385

Add IsReadableStreamDisturbed predicate #385

Conversation

tyoshino commented Aug 3, 2015

tyoshino commented Aug 3, 2015

yutakahirano commented Aug 3, 2015

domenic Aug 3, 2015

Choose a reason for hiding this comment

tyoshino Aug 4, 2015

Choose a reason for hiding this comment

domenic commented Aug 3, 2015

tyoshino commented Aug 4, 2015

domenic commented Aug 4, 2015

tyoshino commented Aug 5, 2015

domenic commented Aug 5, 2015

domenic commented Aug 17, 2015

yutakahirano commented Aug 18, 2015

domenic commented Aug 18, 2015

domenic commented Aug 18, 2015

domenic commented Aug 18, 2015

calvaris commented Aug 19, 2015

domenic commented Aug 19, 2015

yutakahirano commented Aug 19, 2015

domenic commented Aug 19, 2015

yutakahirano commented Aug 19, 2015

domenic commented Aug 19, 2015

domenic commented Aug 20, 2015

tyoshino commented Aug 24, 2015

domenic commented Aug 25, 2015

tyoshino Aug 26, 2015

Choose a reason for hiding this comment

tyoshino Aug 26, 2015

Choose a reason for hiding this comment

tyoshino Aug 26, 2015

Choose a reason for hiding this comment

tyoshino commented Aug 26, 2015

domenic commented Aug 27, 2015

yutakahirano commented Sep 7, 2015

yutakahirano commented Sep 11, 2015

wanderview commented Sep 11, 2015

yutakahirano commented Sep 11, 2015

wanderview commented Sep 11, 2015