WritableStream abort logic clean up #655

tyoshino · 2017-01-16T09:04:35Z

This patch factors out the code to handle fulfillment/rejection of sink write and sink close operation into separate methods to make it easier to follow the code and check the correctness of the logic.

This patch also clarifies the interface boundary, i.e. factoring out operations that are exposed by the WritableStream to controllers.

This PR is recreation of #640.

tyoshino · 2017-01-16T09:07:12Z

This patch doesn't include any change on the spec text, yet. I'll change the spec text once we agree on this change.

tyoshino · 2017-01-16T09:33:45Z

I'd like to summarize the current (or new for some places where I made change) state transition rule of the WritableStream. There're some Qs I'd like to discuss.

Until a WS starts, no write request becomes "a pending write"
Once a WS starts, the head write request may picked to be a pending write if allowed by the backpressure, etc.
- Q: Is this "one pending write at a time" restriction a part of WritableStream's semantics or WritableStreamDefaultController's semantics?
writer.closed change is delayed if there's any pending write or pending close until the pending write/close finishes
writer.ready gets updated immediately in response to controller.error() and writer.abort()
controller.error() call made during there's pending abort wins the abort.
- writer.abort() gets rejected with the error given to the controller.error()
- Q: Do you like this new behavior?
writer.write() gets rejected with different reasons before and after completion of sink write rejection
- That is, for example, if we call writer.abort() and then writer.writer() when there's one pending writer.write() and two writer.write()s in _writeRequests and then the pending writer.write() finishes by rejection of sink write, the two writer.write()s in _writeRequests get rejected with the result of sink write while the writer.write() after writer.abort() gets rejected with new TypeError('Aborted')

Implementation notes

For logic simplicity, state processing e.g. calculation of readyPromiseIsPending is done at some early timing before the method starts modifying the state variables even if it increases the code size.
Steps in the WSFinish(?:Pending)?(Close|Write) methods are ordered so that it's easy to compare them to make it easier to find missing operation.

tyoshino · 2017-01-16T09:45:23Z

Fixes #656

tyoshino · 2017-01-16T11:14:42Z

To add to #655 (comment),

Error priority (higher first) for .closed:

controller.error()
sink write rejection
writer.abort()

Error priority (higher first) for .ready: First invocation of these three above.

You can see this difference by looking at the recently added unit tests.

ricea · 2017-01-16T11:53:43Z

reference-implementation/to-upstream-wpts/writable-streams/aborting.js

+    return Promise.all([
+      promise_rejects(t, new TypeError(), writePromise2, 'writePromise2 must be rejected'),
+      promise_rejects(t, new TypeError(), writer.ready, 'writer.ready must be rejected after abort()'),
+      flushAsyncEvents()


Missing return

How? The return statement is 3 lines above this.

Sorry, I misread it. Never mind.

ricea · 2017-01-16T11:59:51Z

reference-implementation/to-upstream-wpts/writable-streams/aborting.js

+    writer.releaseLock();
+
+    return Promise.all([
+      promise_rejects(t, new TypeError(), writer.ready, 'writer.ready must be reset to be rejected with a TypeError'),


Nitpick: the assertion description implies that it verifies more than it actually does. If we want to actually check that it's been reset, we have to save the previous value and verify it is not equal.

Ah, good point. Resetting is internal thing. What we should verify is that the promises are rejected with an error which indicates "already released". Fixed.

domenic

Answering the Qs:

Q: Is this "one pending write at a time" restriction a part of WritableStream's semantics or WritableStreamDefaultController's semantics?

I don't have a strong opinion on this.

Q: Do you like this new behavior?

This seems correct to me.

Overall this is a big change, and it will be helpful if you could draft the commit message explaining all that it changes and what the benefits are. The OP has some of the non-public facing changes but it sounds like there are also some changes to the behavior during error conditions.

I also wonder how this will tie into #634, both the normative change there and the suggested style change to use the queue more.

domenic · 2017-01-17T22:25:09Z

reference-implementation/to-upstream-wpts/writable-streams/close.js

+
+    const events = [];
+    return Promise.all([
+      closePromise.then(() => {


I would redo these as

promise_rejects(t, new TypeError(), error1, 'closePromise must reject with the error returned from the sink\'s close method') .then(() => events.push('closePromise'))

Same for all tests. In general using assert_unreached for promises seems like a code smell.

OK. Did you mean that I should redo rejection expecting ones e.g. at L259 as your example? Just to make sure.

domenic · 2017-01-17T22:26:52Z

reference-implementation/to-upstream-wpts/writable-streams/aborting.js

+  });
+
+  let writePromise;
+  let writePromiseRejected = false;


In general I prefer arrays of events and assert_array_equals to boolean flags, but it's not a big deal.

Changed to use an array

tyoshino · 2017-01-20T11:24:15Z

Addressed the comments and rewrote the description of assertions.

I'll update the PR description to list all the visible behavior change.

ricea

Logic changes lgtm.

ricea · 2017-01-20T14:04:57Z

reference-implementation/lib/writable-stream.js

@@ -109,6 +109,25 @@ function IsWritableStreamLocked(stream) {
  return true;
 }

+function WritableStreamEnsureReadyPromiseRejectedWith(stream, error, isPending) {


I feel that this is actually a WritableStreamDefaultWriter operation. The only place it's called without a stream._writer !== undefined check is inside DefaultWriter itself.

WritableStreamDefaultWriterEnsureReadyPromiseRejectedWith is a very long name but I think I can live with it.

Good catch. Fixed.

ricea · 2017-01-20T14:48:26Z

reference-implementation/to-upstream-wpts/writable-streams/aborting.js

+      flushAsyncEvents()
+    ]);
+  }).then(() => {
+    const writePromise3 = writer.write('a');


I would find it more emotionally satisfying if you checked the contents of events again here. Adding all those strings to it created a powerful sense of anticipation. Otherwise you could have just used a counter.

Yeah, added.

ricea · 2017-01-20T14:54:17Z

reference-implementation/to-upstream-wpts/writable-streams/aborting.js

+      flushAsyncEvents()
+    ]);
+  }).then(() => {
+    const writePromise4 = writer.write('a');


Again, it would make me happy if you could check events here.

ricea · 2017-01-20T14:56:01Z

reference-implementation/to-upstream-wpts/writable-streams/aborting.js

+      flushAsyncEvents()
+    ]);
+  }).then(() => {
+    assert_array_equals(events, [], 'writePromise, abortPromise and writer.closed must be not fulfilled/rejected yet');


s/be not/not be/. Also below.

ricea · 2017-01-20T14:57:24Z

reference-implementation/to-upstream-wpts/writable-streams/aborting.js

+      flushAsyncEvents()
+    ]);
+  }).then(() => {
+    assert_array_equals(events, [], 'writePromise and writer.closed must be rejected yet');


"must not be rejected". Also below.

Fixed. Also changed the phrase to "fulfilled/rejected". writePromise will fulfill in this test case.

ricea · 2017-01-20T14:59:31Z

reference-implementation/to-upstream-wpts/writable-streams/aborting.js

+  }).then(() => {
+    assert_array_equals(events, [], 'writePromise and writer.closed must be rejected yet');
+
+    abortPromise = writer.abort(error1);


Also put .catch(() => event.push(...)) here so we can check it isn't called too early?

Thanks for finding this. Yes, it's important. Actually, in this test case, abort() fails immediately since it checks the state.

ricea · 2017-01-20T15:04:39Z

reference-implementation/to-upstream-wpts/writable-streams/close.js

@@ -213,4 +219,96 @@ promise_test(() => {
  });
 }, 'the promise returned by async abort during close should resolve');

+// Though the order of the promises is not important, we're checking it for interoperability. We can change the order


I suggest "arbitrary" rather than "not important". Also, maybe "order in which the promises are fulfilled or rejected"?

Thanks. Updated.

tyoshino · 2017-01-20T15:41:50Z

Oops, sorry. Some review comments are not yet addressed correctly. Fixing them.

Spec change: whatwg/streams#655

tyoshino · 2017-01-20T16:04:33Z

Rebased

wpt PR: web-platform-tests/wpt#4587

Here're links for checking the diff before rebasing:

addressed ricea's comment: 26ba2c5
addressed domenic's comment: ca44eaa
addressed ricea's comment: 1156eef

domenic · 2017-01-20T20:16:25Z

The changes look good to me, mainly based on the increase test coverage, but I'm still looking forward to a description of the visible behavior changes :)

ricea · 2017-01-23T04:21:29Z

I would prefer the changes to the standard text to be in the same commit because I'm nervous about them getting out-of-sync.

tyoshino · 2017-01-23T04:51:00Z

Yes. I'm going to make the spec text sync. I didn't mean that I'm going to have two separate commits in the comment.

I created a draft of the commit message to describe all the details in response to Domenic's request in weekend, but haven't done yet. I'm going to get it done today.

Adam, are you fine with the change (reference impl)?

ricea · 2017-01-23T05:08:30Z

Yes. writable-stream.js lgtm.

tyoshino · 2017-01-23T05:42:18Z

~~[STILL DRAFT]~~

The following commit message drafted here has been moved to the actual commit

This patch factors out the logic to handle fulfillment/rejection of sink.write() and sink.close() operation into separate methods to make it easier to follow the code and check correctness while fixing a few bugs.

This patch also clarifies the interface boundary, i.e. factoring out operations that are exposed by the WritableStream to controllers.

This PR is recreation of #640.

Principles behind this change:

writer.closed rejection is delayed while there's any pending writer.write() or writer.close()
- rejection reason picking order:
  - 1. the error passed to controller.error() made during the pending operation if made
  - 1. the rejection reason of the pending operation if rejected
  - 1. a TypeError indicating a writer.abort() call was made if made
- the picked reason will be set to [[storedError]]
writer.ready rejects immediately in response to controller.error() and writer.abort() with the error passed to the method
writer.write() calls made after writer.abort() rejects with:
- if it's made before writer.abort() call and is staying in [[writeRequests]]: [[storedError]]
- if it's made after writer.abort() but before controller.error() if any: an error indicating a writer.abort() call was made
- if ti's made after controller.error() if any: the error passed to the controller.error()
- if it's made after the pending operation finishes: [[storedError]] which stores the reason picked by following the steps above
writer.abort() fulfills if the pending writer.close() succeeds, and writer.closed rejects
- NEW Q: This patch didn't change this behavior but changed the message.

Bug fixes:

Fixed sink.close() rejection handling logic to reject writer.closed even if the stream has been writer.abort()-ed or controller.error()-ed WritableStream: .closed not rejected/resolved after pending close completion if aborted #656
- Before this fix, WritableStreamDefaultControllerErrorIfNeeded() was used which is no-op if [[state]] has already been set to "errored"
NEW Fixed writer.releaseLock() to reject writer.closed when there's a pending write or close since closed rejection is delayed. Just checking [[pendingAbortRequest]] is not enough.

Other visible changes:

Now writer.abort() doesn't set the [[state]] of the stream to "errored" when there's pending sink.write() or sink.close() but prevents further operations on writer by setting [[pendingAbortRequest]] to non-undefined value.
- Note that writer.ready still rejects immediately
- Now controller.error() succeeds even after writer.abort().
  - So, writer.abort() and writer.closed reject with the error passed to controller.error()
  - Note that writer.ready is kept rejected with an error indicating abort even after controller.error().
Now sink.close() rejection handling logic doesn't change the rejection reason of writer.ready if it's already rejected by writer.abort() or controller.error()
- Q: Is this ok? When a writer is re-obtained, the ready promise is set to be rejected with the latest error.
Now sink.write() rejection handling logic doesn't change the [[storedError]] to the rejection reason of sink.write() if it's already controller.error()-ed
- NEW same about writer.ready

Fulfillment/rejection order changes:

Rejection order on sink.write() completion:
- Before: write(), closed, abort()
- After: write(), abort(), closed
Rejection order on sink.write() rejection:
- Before: write(), closed, abort(), ready
- After: write(), abort(), ready, closed
- Q: Worse?
controller.error() rejects:
- Before: closed then ready
- After: ready then closed
NEW writer.abort() rejects
- Before: closed, ready
- After: ready, closed

Refactoring:

Refactored writer.abort() finalization steps to clarify where a certain process is scheduled to run
Factored out [[pendingWriteRequest]] setting logic as a WritableStream abstract operation exposed to controllers
Removed redundant [[queue]] clearing from sink.write() rejection handling logic
Factored out:
- WritableStreamDefaultControllerUpdateBackpressureIfNeeded()
- WritableStreamRejectClosedPromiseIfAny()
- WritableStreamDefaultWriterEnsureReadyPromiseRejectedWith()
Call what's needed directly in sink.write() rejection than using WritableStreamDefaultControllerErrorIfNeeded()
NEW Don't clear [[queue]] on sink.close() rejection. It's guaranteed to be empty as we're processing closure.
Finish calculating conditions at the top of methods to avoid thinking about where a certain state has been updated or not yet (e.g. [[error]])
Have redundant call to promise rejection helper method if needed to keep rejection order consistent

ricea · 2017-01-23T06:20:07Z

sink.close() rejection doesn't change the rejection reason of writer.ready if it's already rejected due to writer.abort() or controller.error()

Q: Is this ok? When a writer is reobtained, the ready promise is set to be rejected with the latest error.

I think it would be more consistent if the rejection reason changed, but unlikely to make any practical difference to anyone. So not worth making the logic more complicated for.

ricea · 2017-01-23T06:24:49Z

Rejection order on sink.write() rejection
- Before: write(), closed, abort(), ready
- After: write(), abort(), ready, closed
- Q: Worse?

I like it better actually. Ready normally rejects ASAP to indicate that writing now is useless, whereas closed rejects later so that it can provide a guarantee that everything has reached a stable state. Here both things happen together, but it's good to be consistent. Ideally ready would reject first or second, but it seems unimportant.

tyoshino · 2017-01-23T07:27:11Z

I don't understand the reason for this change. The promises from Writer methods can remain pending after the the Writer is detached, but until now the .closed and .ready promises have always been rejected immediately.

.closed is not. .ready was rejected immediately and still.

tyoshino · 2017-01-23T07:28:30Z

Almost finished. I've updated #655 (comment) by adding notes about the fixes made after the post was made with NEW mark so that ricea can comment to the new ones easily.

tyoshino · 2017-01-23T07:29:25Z

I think it would be more consistent if the rejection reason changed, but unlikely to make any practical difference to anyone. So not worth making the logic more complicated for.

Thanks. ricea. I'll keep this unaddressed.

ricea · 2017-01-24T08:56:31Z

index.bs

-       with a fulfillment handler that returns ! WritableStreamDefaultControllerAbort(_controller_, _reason_).
-    1. Otherwise, return _stream_.[[pendingAbortRequest]].
-  1. Return ! WritableStreamDefaultControllerAbort(_controller_, _reason_).
+  1. If _state_ is `"writable"` and ! WritableStreamDefaultControllerGetBackpressure(_stream_.[[writableStreamController]]) is *true*, let _readyPromiseIsPending_ be *true*.


This construction bothers me a bit because my brain interprets it as the declaration of the variable happening inside the if statement. However, I don't have a better idea.

Feel free to disregard this comment.

Yeah, as it's uncommon idiom, I wondered how it looks to you reviewers.

Given that it made you think a bit, I rewrote it by separating it into let and updating statement. I think it's ok. Thanks

ricea · 2017-01-24T08:57:22Z

index.bs

+  1. Let _abortRequest_ be _stream_.[[pendingAbortRequest]].
+  1. Set _stream_.[[pendingAbortRequest]] to *undefined*.
+  1. Let _promise_ be ! WritableStreamDefaultControllerAbort(_controller_, _abortRequest_.[[reason]]).
+  1. Transform _promise_ with a fulfillment handler which takes the argument _result_ and <a>resolves</a> _abortRequest_.[[promise]] with _result_ and a rejection handler which takes the argument _reason_ and <a>rejects</a> _abortRequest_.[[promise]] with _reason_.


Linkify Transform? #665

Good catch! Fixed

ricea · 2017-01-24T09:02:37Z

index.bs

+  1. Set _stream_.[[state]] to `"errored"`.
+  1. Set _stream_.[[storedError]] to _reason_.
+  1. If _wasAborted_ is *false* and _stream_.[[writer]] is not *undefined*,
+    1. WritableStreamDefaultWriterEnsureReadyPromiseRejectedWith(_stream_.[[writer]], _reason_, _readyPromiseIsPending_).


I think this can be on the same line as the if condition.

Also there should be a '!' before WritableStreamDefaultWriterEnsureReadyPromiseRejectedWith.

Yeah. Moved and added "perform !"

ricea · 2017-01-24T09:07:28Z

index.bs

+  1. Set _stream_.[[state]] to `"errored"`.
+  1. Set _stream_.[[storedError]] to _reason_.
+  1. If _wasAborted_ is *false* and _stream_.[[writer]] is not *undefined*,
+    1. WritableStreamDefaultWriterEnsureReadyPromiseRejectedWith(_stream_.[[writer]], _reason_, _readyPromiseIsPending_).


Again can be on the same line and missing "Perform !"

ricea · 2017-01-24T09:40:14Z

index.bs

@@ -3156,15 +3261,14 @@ nothrow>WritableStreamDefaultWriterRelease ( <var>writer</var> )</h4>
  1. Assert: _stream_.[[writer]] is _writer_.
  1. Let _releasedError_ be a new *TypeError*.
  1. Let _state_ be _stream_.[[state]].
-  1. If _state_ is `"writable"` or `"closing"`, or _stream_.[[pendingAbortRequest]] is not *undefined*, <a>reject</a>
+  1. Let _controller_ be _stream_.[[writableStreamController]].
+  1. If _state_ is `"writable"` or `"closing"` or _controller_.[[inClose]] is *true* or _controller_.[[writing]] is *true*, or _stream_.[[pendingAbortRequest]] is not *undefined*, <a>reject</a>


The stream.[[pendingAbortRequest]] condition doesn't exist in the reference implementation (unless I'm looking at the wrong version).

Great catch. Right, I've removed the condition but forgot to do so on the spec text.

Done.

ricea · 2017-01-24T09:49:04Z

index.bs

-  1. If _state_ is `"writable"` and ! WritableStreamDefaultControllerGetBackpressure(_stream_.[[writableStreamController]]) is *true*, let _readyPromiseIsPending_ be *true*.
-  1. Otherwise, let _readyPromiseIsPending_ be *false*.
+  1. Let _readyPromiseIsPending_ be *false*.
+  1. If _state_ is `"writable"` and ! WritableStreamDefaultControllerGetBackpressure(_stream_.[[writableStreamController]]) is *true*, set _readyPromiseIsPending_ to *true*.


Much better! Thank you!

ricea · 2017-01-24T09:51:20Z

Reviewed everything up to d560c9b.

tyoshino · 2017-01-24T09:59:56Z

Addressed the review comments and wrapped long lines where possible.

ricea · 2017-01-24T10:01:09Z

lgtm. I will leave decision on the wrapping to @domenic.

domenic · 2017-01-24T22:40:42Z

I pushed some style tweaks (mostly line wrapping) for the spec text. LGTM now. Remember we want to land tests in WPT first then update this branch. This could be a bit tricky given the changes in web-platform-tests/wpt#4589 so if you want me to handle it let's just make sure to sign off on web-platform-tests/wpt#4587 then I can do both merges.

Spec change: whatwg/streams#655

- Fixes the test description of one test - Modified some tests to check promise fulfillment/rejection order - Added tests to check the order and timing of promise fulfillment/rejection when writer.abort() is called while there's a pending writer.write() or writer.close() - Added tests to check that writer.releaseLock() doesn't hit assertions when called while there's a pending writer.write() or writer.close() - Added tests to check an error from which of controller.error(), sink.write()/close() or writer.abort()) is used for rejecting writer.write()/close() Corresponding spec side change: whatwg/streams#655

tyoshino · 2017-01-25T06:43:16Z

Thank you domenic for the commit with great catches in bac156e. LGTM.

Remember we want to land tests in WPT first then update this branch. This could be a bit tricky given the changes in web-platform-tests/wpt#4589

I've updated run-web-platform-tests.js in 59820d5 as you suggested in the OP of the PR, and updated submodule in b6a2cbf.

so if you want me to handle it let's just make sure to sign off on web-platform-tests/wpt#4587 then I can do both merges.

Thanks for merging.

tyoshino · 2017-01-25T06:44:49Z

push is failing on travis at deploy.sh

tyoshino · 2017-01-25T06:55:20Z

Looks bikeshed API is down. Investigating.

tyoshino · 2017-01-25T07:04:42Z

Looks this is due to speced/bikeshed#843

tyoshino · 2017-01-25T07:12:13Z

https://tabatkins.github.io/bikeshed/#metadata-inline-github-issues is not yet updated.

According to the error message, we can choose from 'title', 'full' or false/no/n/off.

Tentatively changed it to title from true in 83dbf13.

This patch factors out the logic to handle fulfillment/rejection of `sink.write()` and `sink.close()` operation into separate methods to make it easier to follow the code and check correctness while fixing a few bugs. This patch also clarifies the interface boundary, i.e. factoring out operations that are exposed by the WritableStream to controllers. This PR is recreation of #640. Principles behind this change: - `writer.closed` rejection is delayed while there's any pending `writer.write()` or `writer.close()` - rejection reason picking order: - 1. the error passed to `controller.error()` made during the pending operation if made - 2. the rejection reason of the pending operation if rejected - 3. a TypeError indicating a `writer.abort()` call was made if made - the picked reason will be set to `[[storedError]]` - `writer.ready` rejects immediately in response to `controller.error()` and `writer.abort()` with the error passed to the method - `writer.write()` calls made after `writer.abort()` rejects with: - if it's made before `writer.abort()` call and is staying in `[[writeRequests]]`: `[[storedError]]` - if it's made after `writer.abort()` but before `controller.error()` if any: an error indicating a `writer.abort()` call was made - if it's made after `controller.error()` if any: the error passed to the `controller.error()` - if it's made after the pending operation finishes: `[[storedError]]` which stores the reason picked by following the steps above - `writer.abort()` fulfills if the pending `writer.close()` succeeds, and `writer.closed` rejects - Note: This patch didn't change this behavior but changed the message. Bug fixes: - Fixed `sink.close()` rejection handling logic to reject `writer.closed` even if the stream has been `writer.abort()`-ed or `controller.error()`-ed #656 - Before this fix, `WritableStreamDefaultControllerErrorIfNeeded()` was used which is no-op if `[[state]]` has already been set to `"errored"` - Fixed `writer.releaseLock()` to reject `writer.closed` when there's a pending write or close since closed rejection is delayed. Just checking `[[pendingAbortRequest]]` is not enough. Other visible changes: - Now `writer.abort()` doesn't set the `[[state]]` of the stream to `"errored"` when there's pending `sink.write()` or `sink.close()` but prevents further operations on writer by setting `[[pendingAbortRequest]]` to non-**undefined** value. - Note that `writer.ready` still rejects immediately - Now `controller.error()` succeeds even after `writer.abort()`. - So, `writer.abort()` and `writer.closed` reject with the error passed to `controller.error()` - Note that `writer.ready` is kept rejected with an error indicating abort even after `controller.error()`. - Now `sink.close()` rejection handling logic doesn't change the rejection reason of `writer.ready` if it's already rejected by `writer.abort()` or `controller.error()` - Question to be discussed: Is this ok? When a writer is re-obtained, the ready promise is set to be rejected with the latest error. - Now `sink.write()` rejection handling logic doesn't change the `[[storedError]]` to the rejection reason of `sink.write()` if it's already `controller.error()`-ed - Same about `writer.ready` Fulfillment/rejection order changes: - Rejection order on `sink.write()` completion: - Before: write(), closed, abort() - After: write(), abort(), closed - Rejection order on `sink.write()` rejection: - Before: write(), closed, abort(), ready - After: write(), abort(), ready, closed - `controller.error()` rejects: - Before: closed then ready - After: ready then closed - `writer.abort()` rejects - Before: closed, ready - After: ready, closed Refactoring: - Refactored `writer.abort()` finalization steps to clarify where a certain process is scheduled to run - Factored out `[[pendingWriteRequest]]` setting logic as a WritableStream abstract operation exposed to controllers - Removed redundant `[[queue]]` clearing from `sink.write()` rejection handling logic - Factored out: - `WritableStreamDefaultControllerUpdateBackpressureIfNeeded()` - `WritableStreamRejectClosedPromiseIfAny()` - `WritableStreamDefaultWriterEnsureReadyPromiseRejectedWith()` - Call what's needed directly in `sink.write()` rejection than using `WritableStreamDefaultControllerErrorIfNeeded()` - Don't clear `[[queue]]` on `sink.close()` rejection. It's guaranteed to be empty as we're processing closure. - Finish calculating conditions at the top of methods to avoid thinking about where a certain state has been updated or not yet (e.g. `[[error]]`) - Have redundant call to promise rejection helper method if needed to keep rejection order consistent Closes #639, #656 This patch also includes some additional work as follows: - Update worker excluding pattern in test runner - web-platform-tests/wpt#4589 has changed the naming rule of streams test files. - Update the condition in run-web-platform-tests.js to exclude worker tests to follow the new rule. - Sync wpt submodule to the current HEAD to run the updated tests - Fix bikeshed option: Inline Github Issues - bikeshed API has been updated to reject true for the Inline Github Issues option. - To pass the "push" check on Travis CI, we need to fix this to make deploy.sh run successfully - Tentatively set it to one of the new valid values, "title".

tyoshino · 2017-01-25T07:23:41Z

Squashed and updated the commit message to explain the additional works required for submission. The last commit before this was 83dbf13.

tyoshino · 2017-01-25T07:27:55Z

Done. Finally. Thank you for your review.

tyoshino mentioned this pull request Jan 16, 2017

Factor out rejection handling logic from WritableStreamDefaultControllerProcessClose() #640

Closed

tyoshino force-pushed the writablestreamrefactoring branch from 0058ac5 to 49c0d29 Compare January 16, 2017 09:05

tyoshino added writable streams bug labels Jan 16, 2017

ricea reviewed Jan 16, 2017

View reviewed changes

domenic reviewed Jan 17, 2017

View reviewed changes

This was referenced Jan 19, 2017

Move to the upstream versions of WPTs #662

Merged

Do not allow abort to happen when a close is about to happen #634

Closed

ricea reviewed Jan 20, 2017

View reviewed changes

tyoshino added a commit to tyoshino/web-platform-tests that referenced this pull request Jan 20, 2017

Update WritableStream tests

d9466cf

Spec change: whatwg/streams#655

tyoshino mentioned this pull request Jan 20, 2017

Update WritableStream tests web-platform-tests/wpt#4587

Merged

tyoshino force-pushed the writablestreamrefactoring branch from ca44eaa to a0ac58b Compare January 20, 2017 16:03

tyoshino mentioned this pull request Jan 24, 2017

Don't touch controller directly in WritableStreamDefaultWriterRelease() #669

Closed

ricea reviewed Jan 24, 2017

View reviewed changes

tyoshino added a commit to tyoshino/web-platform-tests that referenced this pull request Jan 25, 2017

Update WritableStream tests

83a54c4

Spec change: whatwg/streams#655

tyoshino force-pushed the writablestreamrefactoring branch from 83dbf13 to 673d1f7 Compare January 25, 2017 07:22

tyoshino merged commit c6c0a6e into master Jan 25, 2017

tyoshino deleted the writablestreamrefactoring branch January 25, 2017 07:27

This was referenced Jan 25, 2017

Update worker excluding code in run-web-platform-tests.js #673

Closed

Set "Inline Github Issues bikeshed" option to appropriate value #674

Closed

Factor out condition to check pending operation #672

Merged

tschneidereit mentioned this pull request Feb 11, 2017

Writable stream spec fixes #681

Merged

domenic mentioned this pull request Jan 6, 2022

Should the abort result reflect the close result? #1203

Open

WritableStream abort logic clean up #655

WritableStream abort logic clean up #655

Conversation

tyoshino commented Jan 16, 2017

tyoshino commented Jan 16, 2017 • edited Loading

tyoshino commented Jan 16, 2017

tyoshino commented Jan 16, 2017

tyoshino commented Jan 16, 2017

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 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

tyoshino commented Jan 20, 2017

ricea 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

tyoshino commented Jan 20, 2017

tyoshino commented Jan 20, 2017 • edited Loading

domenic commented Jan 20, 2017

ricea commented Jan 23, 2017

tyoshino commented Jan 23, 2017 • edited Loading

ricea commented Jan 23, 2017

tyoshino commented Jan 23, 2017 • edited Loading

ricea commented Jan 23, 2017

ricea commented Jan 23, 2017

tyoshino commented Jan 23, 2017

tyoshino commented Jan 23, 2017

tyoshino commented Jan 23, 2017

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

ricea commented Jan 24, 2017

tyoshino commented Jan 24, 2017

ricea commented Jan 24, 2017

domenic commented Jan 24, 2017

tyoshino commented Jan 25, 2017

tyoshino commented Jan 25, 2017

tyoshino commented Jan 25, 2017

tyoshino commented Jan 25, 2017

tyoshino commented Jan 25, 2017

tyoshino commented Jan 25, 2017

tyoshino commented Jan 25, 2017

tyoshino commented Jan 16, 2017 •

edited

Loading

tyoshino commented Jan 20, 2017 •

edited

Loading

tyoshino commented Jan 23, 2017 •

edited

Loading

tyoshino commented Jan 23, 2017 •

edited

Loading