Unified error handling for WritableStream #721

ricea · 2017-03-30T16:57:38Z

Previously, controller.error() and errors from strategy size() could transition
the stream state to change to 'errored' while an underlying sink operation
was in flight.

Only change to 'errored' while no underlying sink operations are in flight,
consistent with abort(). The first error to happen sets the [[storedError]]
internal slot, regardless of type.

Other behavioural changes:

The writer.closed promise now doesn't reject until the underlying sink
abort() method has completed.
writer.close(), writer.abort() and writer.closed promises now consistently
don't fulfill or reject until no underlying sink methods are executing or will
be executed. Exceptions:
- Duplicate calls to close() or abort() still reject immediately.
- writer.closed still rejects when writer.releaseLock() is called.
writer.write(), writer.close() and writer.abort() consistently return the
status of the underlying sink operation, provided it executes.
It is no longer possible to error the stream once underlying sink close()
has been called.
The controller parameter to the underlying sink close() method is no
longer useful and has been removed.
Duplicate calls to controller.error() are now a no-op.
Calls to controller.error() while the stream is erroring, errored or
closed are no a no-op.

Major implementation changes:

There is a new "erroring" state. The state synchronously changes
from "writable" to "erroring" when an error condition is triggered,
then changes to "errored" once the underlying sink operation
has finished.
The stream.[[pendingAbortRequest]] Record has an additional
boolean field [[wasAlreadyErrored]]. When it is true the underlying
sink abort() method will not be called.

ricea · 2017-03-30T17:01:35Z

Needs a proper commit message writing, but you can see the behaviour changes from the tests. PTAL.

The standard text changes are not written yet.

domenic

Looking pretty great... will review tests next...

domenic · 2017-03-31T10:20:23Z

reference-implementation/lib/writable-stream.js


  assert(state === 'writable', 'state must be writable');

  const error = new TypeError('Requested to abort');
-  if (stream._pendingAbortRequest !== undefined) {
+  if (stream._pendingAbortRequest === true) {


Is it a boolean or a promise?

It's a promise. Well, actually a record containing a promise and a reason. Great, now I need to work out whether this is a test gap or a no-op.

npm run coverage may help!

It was a no-op. There was a correct check above. I have removed the redundant check.

Coverage says the real check was being tested, but I can't find the test that does it. Maybe it is a piping one? Anyway, I added an explicit test for double aborting just to make it clear.

domenic · 2017-03-31T10:29:22Z

reference-implementation/lib/writable-stream.js

@@ -405,7 +383,7 @@ class WritableStreamDefaultWriter {
    const state = stream._state;

    if (state === 'writable') {
-      if (stream._pendingAbortRequest !== undefined) {
+      if (stream._pendingError === true) {
        const error = new TypeError('Requested to abort');


Error message needs tweaking. Or should we use _storedError? I think that would be consistent with e.g. write().

It's also consistent with the 'errored' state, so I've gone with that.

domenic · 2017-03-31T10:30:19Z

reference-implementation/lib/writable-stream.js

@@ -567,7 +542,7 @@ function WritableStreamDefaultWriterClose(writer) {
    stream._closeRequest = closeRequest;
  });

-  if (stream._backpressure === true) {
+  if (stream._backpressure === true && !stream._pendingError) {


Nit: === false to match spec text

_pendingError is gone in favour of state 'erroring'.

domenic · 2017-03-31T10:30:58Z

reference-implementation/lib/writable-stream.js

-  }
-  writer._closedPromise.catch(() => {});
+  // The state transitions to "errored" before the sink abort() method runs, but the writer.closed promise is not
+  // rejected until afterwards. This means that simply testing state will not work


Nit: period at end of sentence.

domenic · 2017-03-31T10:31:42Z

reference-implementation/lib/writable-stream.js

@@ -711,8 +691,12 @@ class WritableStreamDefaultController {
      throw new TypeError(
        'WritableStreamDefaultController.prototype.error can only be used on a WritableStreamDefaultController');
    }
+    const stream = this._controlledWritableStream;
+    if (stream._pendingError) {


Nit: === true

domenic · 2017-03-31T10:32:38Z

reference-implementation/lib/writable-stream.js

-        } else {
-          WritableStreamHandleAbortRequestIfPending(stream);
-        }
-
        // This is a no-op if the stream was errored above.


Is this comment still accurate?

It's no longer meaningful. I have removed it.

domenic · 2017-03-31T10:32:56Z

reference-implementation/lib/writable-stream.js

-        assert(stream._state === 'writable' || stream._state === 'errored');
-        WritableStreamDefaultControllerErrorIfNeeded(this, r);
-        WritableStreamRejectAbortRequestIfPending(stream);
+        assert(stream._state === 'writable');


Maybe this assert should be in both branches now?

domenic · 2017-03-31T11:38:26Z

reference-implementation/lib/writable-stream.js

@@ -12,6 +12,9 @@ const ErrorSteps = Symbol('[[ErrorSteps]]');
 class WritableStream {
  constructor(underlyingSink = {}, { size, highWaterMark = 1 } = {}) {
    this._state = 'writable';
+
+    // The error that will be reported by new method calls once the state becomes errored. Only set when [[state]] is
+    // 'errored' or [[pendingError]] is true. May be set to an undefined value.


Mentions "pendingError"

domenic · 2017-03-31T11:38:33Z

reference-implementation/lib/writable-stream.js

@@ -42,6 +45,9 @@ class WritableStream {
    // The backpressure signal set by the controller.
    this._backpressure = false;

+    // True if an error is pending on the stream.
+    this._pendingError = false;


No longer necessary

Oops. Removed.

domenic · 2017-03-31T11:39:31Z

reference-implementation/lib/writable-stream.js

-function WritableStreamError(stream, error) {
-  stream._state = 'errored';
-  stream._storedError = error;
+function WritableStreamSetPendingError(stream, reason) {


Maybe this should be renamed WritableStreamBecomeErroring or similar? Might be fine as-is.

Looking at existing abstract operations, it seemed like "WritableStreamErroring" was the most consistent option, so I went with that.

domenic · 2017-03-31T11:53:11Z

code changes lgtm, although @tyoshino always catches good bugs, so I'd really like his review.

domenic · 2017-03-31T11:57:43Z

Oh, I guess changing controller.error() to no-op if erroring/errored would be a good idea actually (per my comment at web-platform-tests/wpt@f196be0#commitcomment-21575312 )

ricea · 2017-04-05T07:52:05Z

The standard changes are ready for review now.

domenic · 2017-04-05T08:33:33Z

Can you rebase/resolve the test conflict so that a branch snapshot gets generated?

domenic

Found some nits :)

domenic · 2017-04-05T08:34:10Z

index.bs

+     ! WritableStreamFinishError(_stream_).
+</emu-alg>
+
+<h4 id="writable-stream-finish-error" aoid="WritableStreamFinishError" nothrow>WritableStreamFinishError


Maybe the abstract ops should be named "WritableStreamStartErroring" and "WritableStreamFinishErroring"?

Makes sense. One that that bugs me is that StartErroring conditionally calls FinishErroring directly. This avoids duplicating the condition in 2 of the callers, and the condition is always true in WritableStreamDealWithRejection so it seems like the right thing, but I still find it confusing.

domenic · 2017-04-05T08:34:46Z

index.bs

+  1. Assert: ! WritableStreamHasOperationMarkedInFlight(_stream_) is *false*.
+  1. Set _stream_.[[state]] to `"errored"`.
+  1. Perform ! _stream_.[[writableStreamController]].[[ErrorSteps]]().
+  1. Let _storedError_ by _stream_.[[storedError]];


Semicolon should be period. "by" should be "be"

domenic · 2017-04-05T08:35:12Z

index.bs

+  1. if _stream_.[[pendingAbortRequest]] is *undefined*,
+    1. Perform !  WritableStreamRejectCloseAndClosedPromiseIfNeeded(_stream_).
+    1. Return.
+  1. Let _abortRequest_ by _stream_.[[pendingAbortRequest]].


"by" should be "be". Might be worth running a regex for "Let ... by".

I found one of these myself which means I had it wrong in three places. 'y' and 'e' aren't even close to each other on the keyboard. Very odd.

domenic · 2017-04-05T08:36:09Z

index.bs

+    1. Return.
+  1. Let _abortRequest_ by _stream_.[[pendingAbortRequest]].
+  1. Set _stream_.[[pendingAbortRequest]] to *undefined*.
+  1. Let _promise_ be stream.[[writableStreamController]].[[AbortSteps]](_abortRequest_.[[reason]]).


Missing ! (although I admit it's unclear whether these internal methods return completion values).

It makes sense to follow the ECMASCRIPT convention here. As long as the reader understands the "?" and "!" notation, they should be able to make sense of it.

domenic · 2017-04-05T08:36:41Z

index.bs

+  1. Let _abortRequest_ by _stream_.[[pendingAbortRequest]].
+  1. Set _stream_.[[pendingAbortRequest]] to *undefined*.
+  1. Let _promise_ be stream.[[writableStreamController]].[[AbortSteps]](_abortRequest_.[[reason]]).
+    1. <a>Upon fulfillment</a> of _promise_, <a>resolve</a> _abortRequest_.[[promise]] with *undefined*.


We're a bit inconsistent but we seem to lean toward not indenting "Upon fulfillment" and "Upon rejection" steps.

Fixed here and in [[StartSteps]].

domenic · 2017-04-05T08:42:31Z

index.bs

  1. Return ! WritableStreamDefaultWriterClose(_writer_).
 </emu-alg>

+<h4 id="writable-stream-default-writer-ensure-closed-promise-rejected"
+aoid="WritableStreamDefaultWriterEnsureClosedPromiseRejected"
+nothrow>WritableStreamDefaultWriterEnsureClosedPromiseRejected( <var>writer</var>, <var>error</var> )</h4>


It occurs to me we could abstract this out into a utility abstract op like EnsureStatePromiseRejected(promise, error). ("State" included to justify marking it as handled.)

Not sure if worth it.

It would be ugly in the reference implementation because of the way the state promises are manipulated by a set of functions there.

domenic · 2017-04-05T08:43:40Z

index.bs

@@ -3582,10 +3568,13 @@ aoid="WritableStreamDefaultControllerAdvanceQueueIfNeeded" nothrow>WritableStrea

 <emu-alg>
  1. Let _stream_ be _controller_.[[controlledWritableStream]].
+  1. if _controller_.[[started]] is *false*, return.


domenic · 2017-04-05T09:20:30Z

I am now confused about the relationship between stream.[[closeRequest]] and writer.[[closedPromise]]. In particular it seems like close() and closed are not returning the same promise anymore---close() returns closeRequest, whereas closed returns closedPromise?

domenic · 2017-04-05T09:23:25Z

WritableStreamFinishAbort does not appear to be under the right heading based on the text

The following abstract operations, unlike most in this specification, are meant to be generally useful by other specifications, instead of just being part of the implementation of this spec’s classes.

EDIT: actually it appears to be dead code.

ricea · 2017-04-05T09:40:53Z

I haven't checked the blame history, but I assume that closeRequest and closedPromise parted ways when getWriter() was added.

I simply failed to delete WritableStreamFinishAbort from the standard text. Now I need to go back and see what else I failed to delete.

tyoshino · 2017-04-05T10:50:11Z

I haven't checked the blame history, but I assume that closeRequest and closedPromise parted ways when getWriter() was added.

Right. It's 5a27534.

After
Before

tyoshino · 2017-04-05T10:55:42Z

I remember that we discussed long time ago a principle whether methods should return a new promise. But I'm not sure if this is related. I no longer remember the motivation. I guess making close() return [[closedPromise]] would also work.

tyoshino · 2017-04-05T11:08:29Z

reference-implementation/lib/writable-stream.js

+  }
+  stream._writeRequests = [];
+
+  if (stream._pendingAbortRequest === undefined) {


Could you please update 'controller.error(), writer.abort() while there is an in-flight write, and then finish the write' test to check that sink.abort() is not called for this (error() then abort()) sequence?

controller.error() calls WritableStreamErroring() to change the state to erroring. WritableStreamAbort() skips WritableStreamErroring() when the state is erroring, but it does set _pendingAbortRequest. It should be rejected (right?) when controller.error() happened before writer.abort(), but it looks it would get processed by the code below.

Good catch. It was indeed broken. I have fixed it. PTAL.

ricea · 2017-04-07T11:17:09Z

Remaining jobs for me to do:

Double-check the spec changes against the reference implementation changes.
Write a more detailed commit message describing the semantic changes.

ricea · 2017-04-10T03:37:03Z

Okay, ready for final reviews now.

tyoshino

Finished reviewing the reference impl.

looks good.

tyoshino · 2017-04-12T11:31:01Z

reference-implementation/lib/writable-stream.js

 }

 function WritableStreamUpdateBackpressure(stream, backpressure) {
-  assert(stream._state === 'writable');
+  assert(stream._state === 'writable' || stream._state === 'erroring');


When does this get called with state set to erroring?

Never. Fixed the assert.

tyoshino · 2017-04-12T11:45:28Z

reference-implementation/lib/writable-stream.js


-  assert(state === 'writable');
+  assert(state === 'writable' || state === 'erroring');


Hmm, what was the rationale to have close() succeed while write() fails when the state is erroring?

writer.close() is useful to wait for the sink to become quiescent. You can also do this with writer.closed, but that would mean writing code like

return writer.close().catch(() => writer.closed);

Also I want this to just work without the developer having to think about it. If there is a developer error like calling close() twice on the same stream then close() will reject immediately, but otherwise it consistently waits for no underlying sink methods to be running.

write() never provides any such guarantee, so rejecting immediately when we know there is no hope of the write() working is the best behaviour.

OK. Makes sense.

tyoshino

lgtm. I'll take a quick look at the spec side too now, but please go ahead merge it.

tyoshino · 2017-04-13T04:30:52Z

reference-implementation/lib/writable-stream.js


-  assert(state === 'writable');
+  assert(state === 'writable' || state === 'erroring');


OK. Makes sense.

domenic · 2017-04-13T04:35:05Z

Commit message nit: "un" should be "an".

Also let's please remember to delete the "(#721)" from the commit message title before merging. Gotta look good on the twitters :).

LGTM!

ricea · 2017-04-13T04:47:35Z

I find it convenient having the pull request linked from the commit title. Why does this look bad on the twitters?

domenic · 2017-04-13T04:50:14Z

The pull request is always linked from the commit page, FWIW.

The problem is it references a mysterious number with no context for anyone consuming the commit messages outside the GitHub UI.

Previously, controller.error() and strategy size() evaluation could cause the stream state to change to 'errored' while un underlying sink operation was in flight. Only change state to 'errored' while no underlying sink operations are in flight. All errors now behave similarly to abort(). The first error to happen sets the [[storedError]] internal slot, regardless of type. A new 'erroring' state is used when an error is pending but the stream has not yet had a change to switch to the 'errored' state. controller.error() is a no-op in the 'erroring' state, making it easier for sinks to use without having to worry about it throwing because abort() has been called.

ricea · 2017-04-13T05:48:25Z

Rebased, squashed and updated web-platform-tests submodule to the merged commit.

Port the standard changes in whatwg/streams#721. There are a number of behavioural changes related to error handling, which are listed at the above URL. This implementation has no known deviations from the standard. The brings this implementation up to parity with whatwg/streams@e7bf929. This CL also removes failing test expectations. Issue 626703 and 711529 cover lines that were removed from TestExpectations. BUG=711254,626703,711529,684543 Review-Url: https://codereview.chromium.org/2823563002 Cr-Commit-Position: refs/heads/master@{#465498}

Port the standard changes in whatwg/streams#721. There are a number of behavioural changes related to error handling, which are listed at the above URL. This implementation has no known deviations from the standard. The brings this implementation up to parity with whatwg/streams@e7bf929. The version of this CL merged to M59 branch 3071 adds failing and time-out test expectations since the tests on hte branch are now out-of-date with respect to the implementation. See http://crbug.com/713664. BUG=711254,684543 Review-Url: https://codereview.chromium.org/2823563002 Cr-Commit-Position: refs/heads/master@{#465498} (cherry picked from commit 6703f4d) Review-Url: https://codereview.chromium.org/2831763003 . Cr-Commit-Position: refs/branch-heads/3071@{#82} Cr-Branched-From: a106f0a-refs/heads/master@{#464641}

An "erroring" state was added to WritableStream in e7bf929 (whatwg#721) but was not included in the description of the [[state]] slot. Add it. Also remove a stray '.' from the description of the [[writeRequests]] slot.

An "erroring" state was added to WritableStream in e7bf929 (#721) but was not included in the description of the [[state]] slot. Add it. Also remove a stray '.' from the description of the [[writeRequests]] slot.

domenic reviewed Mar 31, 2017

View reviewed changes

ricea mentioned this pull request Mar 31, 2017

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

Closed

domenic reviewed Apr 5, 2017

View reviewed changes

ricea force-pushed the writable-stream-all-errors-queued branch from 44cb3d4 to ce6d155 Compare April 5, 2017 08:57

domenic added the writable streams label Apr 5, 2017

tyoshino reviewed Apr 5, 2017

View reviewed changes

tyoshino mentioned this pull request Apr 5, 2017

Editorial: make close() return this.[[closedPromise]] #724

Closed

ricea mentioned this pull request Apr 7, 2017

Writable stream unified error behaviour tests web-platform-tests/wpt#5421

Merged

tyoshino self-assigned this Apr 10, 2017

tyoshino reviewed Apr 12, 2017

View reviewed changes

tyoshino approved these changes Apr 13, 2017

View reviewed changes

ricea added 2 commits April 13, 2017 13:53

Update web-platform-tests submodule

b741cbf

ricea force-pushed the writable-stream-all-errors-queued branch from a1272ab to b741cbf Compare April 13, 2017 05:47

ricea merged commit e7bf929 into whatwg:master Apr 13, 2017

ricea deleted the writable-stream-all-errors-queued branch April 13, 2017 07:30

ricea mentioned this pull request Apr 14, 2017

Calling sink.abort() after a sink method rejection could lead to double cleanup #735

Open

domenic mentioned this pull request Apr 17, 2017

Make writer.abort() fail immediately #715

Closed

ricea mentioned this pull request Sep 22, 2017

Include "erroring" in the list of states for WritableStream #803

Merged


		assert(state === 'writable');
		assert(state === 'writable' \|\| state === 'erroring');

Unified error handling for WritableStream #721

Unified error handling for WritableStream #721

Conversation

ricea commented Mar 30, 2017 • edited Loading

ricea commented Mar 30, 2017

domenic 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

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

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

Choose a reason for hiding this comment

domenic commented Mar 31, 2017

domenic commented Mar 31, 2017

ricea commented Apr 5, 2017

domenic commented Apr 5, 2017

domenic 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic commented Apr 5, 2017

domenic commented Apr 5, 2017 • edited Loading

ricea commented Apr 5, 2017

tyoshino commented Apr 5, 2017 • edited Loading

tyoshino commented Apr 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricea commented Apr 7, 2017 • edited Loading

ricea commented Apr 10, 2017

tyoshino 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyoshino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic commented Apr 13, 2017

ricea commented Apr 13, 2017

domenic commented Apr 13, 2017

ricea commented Apr 13, 2017

ricea commented Mar 30, 2017 •

edited

Loading

domenic commented Apr 5, 2017 •

edited

Loading

tyoshino commented Apr 5, 2017 •

edited

Loading

ricea commented Apr 7, 2017 •

edited

Loading