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

Inconsistency between abort() then error() and error() then abort(), with ongoing close()/write() #693

Closed
tyoshino opened this issue Mar 9, 2017 · 10 comments
Assignees

Comments

@tyoshino
Copy link
Member

tyoshino commented Mar 9, 2017

From #672 (comment)

writer.abort() fails immediately when the writable stream is in the "errored" state regardless of whether or not the stream has an unfinished in-flight close()/write().

OTOH, when a writable stream is errored by controller.error() with a pending writer.abort() and an in-flight close()/write() on it, writer.abort() gets rejected only after the stream finishes the in-flight close()/write().

I found this inconsistent and raised a discussion in the PR.

I agree with ricea that this is a trivial less-important corner case, but sometimes inconsistent behavior confuses users or is working as a hidden source of code complexity. So, I'd like to another scan after landing the PR.

@domenic
Copy link
Member

domenic commented Mar 9, 2017

I guess there are three alternatives:

  1. Make consistent by making both write-abort-error and write-error-abort cause the abort to not reject until the write finishes
  2. Make consistent by making both write-abort-error and write-error-abort reject the abort immediately
  3. Leave as-is

I don't have a strong preference between them. I think it would be good to see if implementing either (1) or (2) would create code simplifications.

@domenic
Copy link
Member

domenic commented Mar 27, 2017

I did some analysis on this and other related matters in https://docs.google.com/document/d/1rOe6CiEHxU7m3_YDh1lv334TxCR6i3X71Fjnem_XKCc/edit?usp=sharing (at the end). I ended up concluding that this is reasonable behavior even from first principles, and isn't (just) a result of our codebase and tests growing organically over time. So, I will close this tomorrow after verifying we have tests for it.

@ricea
Copy link
Collaborator

ricea commented Mar 28, 2017

This is reassuring. Developers will have to work with a simpler mental model than the real thing and obviously it is good if most behaviour is predicted by this model.

@domenic
Copy link
Member

domenic commented Mar 28, 2017

This is tested in aborting.js by the two tests:

  • writer.abort(), controller.error() while there is an in-flight write, and then finish the write
  • controller.error(), writer.abort() while there is an in-flight write, and then finish the write

However I suppose we should also add tests for in-flight close.

@tyoshino
Copy link
Member Author

(1) would remove a few lines from WritableStreamAbort() while adding a few to WritableStreamFinishInFlightClose(), and move some lines in the other handlers, I guess. So, not much gain of simplification, right.

(2) adds a few lines to WritableStreamError() while removing WritableStreamRejectAbortRequestIfPending() from WritableStreamFinishInFlightWriteInErroredState() and WritableStreamFinishInFlightCloseInErroredState(). How does this look to you?

@tyoshino
Copy link
Member Author

Thanks for the analysis, Domenic. It clarified that there's no concern about confusion for users.

So, the only remaining point is possible simplification of the logic.

@tyoshino
Copy link
Member Author

Tried at #715

domenic added a commit to web-platform-tests/wpt that referenced this issue Mar 29, 2017
domenic added a commit to web-platform-tests/wpt that referenced this issue Mar 29, 2017
Closes whatwg/streams#693.

Also cleans up nearby files in response to some post-merge review feedback in #5242 and #5224.
@ricea
Copy link
Collaborator

ricea commented Mar 30, 2017

I am currently attempting to implement (1).

An issue I ran into is that a throwing strategySink can also error the stream without respect for the queue. After discussing it with @domenic, I am going to treat this the same way: first thing which errors the stream "wins" [[storedError]], regardless of what type of error it is.

@tyoshino
Copy link
Member Author

Oh, I'm also trying the plan at #715 (comment).

OK. Just have them in parallel and merge outcome.

@ricea
Copy link
Collaborator

ricea commented Mar 31, 2017

Very belated comment that I have an implementation of (1) at #721 and it's looking good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants