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

Make writer.abort() fail immediately #715

Closed
wants to merge 1 commit into from
Closed

Make writer.abort() fail immediately #715

wants to merge 1 commit into from

Conversation

tyoshino
Copy link
Member

No description provided.

@tyoshino tyoshino added the do not merge yet Pull request must not be merged per rationale in comment label Mar 28, 2017
@tyoshino
Copy link
Member Author

A test impl for #693

@ricea
Copy link
Collaborator

ricea commented Mar 29, 2017

I would prefer not to reject abort() immediately on controller.error() as there may still be operations in progress. I would like there to be a strong guarantee for all of close(), abort() and .closed promises that when they resolve no sink methods are executing, and no further sink methods will be executed.

@tyoshino
Copy link
Member Author

I think the second sentence is what you said in #672 (comment). As I said in #672 (comment), it's not satisfied. Only (1) in Domenic's comment #693 (comment) would satisfy that.

Let me confirm the context. I agree with your point in #672 (comment) but also wondered if correcting it can provide some real benefit in logic simplification and tried that.

Could you please summarize your thoughts again? I understand your point but the current status is that it's not satisfied.

@ricea
Copy link
Collaborator

ricea commented Mar 29, 2017

I wasn't thinking about it like that. You're right, since it is invisible to the user that controller.error() has been called they are not using abort() incorrectly and so can have a reasonable expectation that the invariant would hold.

You are right that I am contradicting what I said in #672 (comment). I have been thinking about what I'd like to write in a "Design Principles" section, and this has convinced me that it is important to provide this guarantee.

But it worries me that this leads to a contradition with Domenic's conclusion in the Writable stream state machine document.

@domenic
Copy link
Member

domenic commented Mar 29, 2017

The basic problem is figuring out what controller.error() means. Is it an immediate "transition to the errored state"? If so that's a bit unusual. But that's been our answer so far.

Or is it similar to abort(), in that it's a request to error when in-flight stuff finishes?

@domenic
Copy link
Member

domenic commented Mar 29, 2017

In particular you end up with parallel questions, like, what if you call controller.error() while sink.close() is running, if sink.close() returns a fulfilled promise. If we switched to semantics similar to abort(), then we'd have to say that this is ignored, as by the time the request-to-error gets executed, the stream has already successfully transitioned to the closed state.

@ricea
Copy link
Collaborator

ricea commented Mar 29, 2017

Or is it similar to abort(), in that it's a request to error when in-flight stuff finishes?

I think this an important question.

My understanding of controller.error():

  1. It is for out-of-band errors that aren't tied to the execution of a particular operation
  2. It is generally expected to be called at times when no operations are executing
  3. If it is called when an operation is executing, we may have a race condition or conflicting information.

I am leaning towards the idea that if controller.error() happens simultaneously with close() or with abort() pending, then it is the close() or abort() that "wins" and determines the eventual state of the stream.

I think I want controller.error() to behave exactly like abort() (but still not return a promise).

Implementation idea: add a flag which takes over the signalling function of [[pendingAbortRequest]]. The flag signals that abort() or controller.error() has been called. [[pendingAbortRequest]] would still be used as storage for the promise returned by abort(). Alternatively, an extra state "erroring" could be added.

@domenic
Copy link
Member

domenic commented Mar 29, 2017

I would be down for that change. It seems likely to simplify things. @tyoshino do you feel up for prototyping it?

@domenic
Copy link
Member

domenic commented Apr 17, 2017

So this has been obsoleted by #721, I am pretty sure. Let me know if I am wrong!

@domenic domenic closed this Apr 17, 2017
@tyoshino tyoshino deleted the 693 branch April 18, 2017 03:45
@tyoshino
Copy link
Member Author

Agreed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge yet Pull request must not be merged per rationale in comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants