Re: [whatwg/streams] WritableStream abort logic clean up (#655)

ricea commented on this pull request.

Logic changes lgtm.

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

> +  }).then(() => {
+    assert_array_equals(events, [], 'writePromise, abortPromise and writer.closed must be not rejected yet');
+
+    rejectWrite(error2);
+
+    return Promise.all([
+      promise_rejects(t, error2, writePromise,
+                      'writePromise must reject with the error returned from the sink\'s write method'),
+      promise_rejects(t, error2, abortPromise,
+                      'abortPromise must reject with the error returned from the sink\'s write method'),
+      promise_rejects(t, error2, writer.closed,
+                      'writer.closed must reject with the error returned from the sink\'s write method'),
+      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.

> +        events, [],
+        'writePromise, abortPromise and writer.closed must be not fulfilled/rejected yet even after '
+            + 'controller.error() call');
+
+    resolveWrite();
+
+    return Promise.all([
+      writePromise,
+      promise_rejects(t, error2, abortPromise,
+                      'abortPromise must reject with the error passed to the controller\'s error method'),
+      promise_rejects(t, error2, writer.closed,
+                      'writer.closed must reject with the error passed to the controller\'s error method'),
+      flushAsyncEvents()
+    ]);
+  }).then(() => {
+    const writePromise4 = writer.write('a');

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

> +    });
+
+    abortPromise = writer.abort(error1);
+    abortPromise.catch(() => {
+      events.push('abortPromise');
+    });
+
+    const writePromise2 = writer.write('a');
+
+    return Promise.all([
+      promise_rejects(t, new TypeError(), writePromise2, 'writePromise2 must reject with an error indicating abort'),
+      promise_rejects(t, new TypeError(), writer.ready, 'writer.ready must reject with an error indicating abort'),
+      flushAsyncEvents()
+    ]);
+  }).then(() => {
+    assert_array_equals(events, [], 'writePromise, abortPromise and writer.closed must be not fulfilled/rejected yet');

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

> +      events.push('writePromise');
+    });
+
+    controller.error(error2);
+
+    const writePromise2 = writer.write('a');
+
+    return Promise.all([
+      promise_rejects(t, new TypeError(), writePromise2,
+                      'writePromise2 must reject with an error indicating the stream has already been errored'),
+      promise_rejects(t, error2, writer.ready,
+                      'writer.ready must reject with the error passed to the controller\'s error method'),
+      flushAsyncEvents()
+    ]);
+  }).then(() => {
+    assert_array_equals(events, [], 'writePromise and writer.closed must be rejected yet');

"must **not** be rejected". Also below.

> +
+    controller.error(error2);
+
+    const writePromise2 = writer.write('a');
+
+    return Promise.all([
+      promise_rejects(t, new TypeError(), writePromise2,
+                      'writePromise2 must reject with an error indicating the stream has already been errored'),
+      promise_rejects(t, error2, writer.ready,
+                      'writer.ready must reject with the error passed to the controller\'s error method'),
+      flushAsyncEvents()
+    ]);
+  }).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?

> @@ -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"?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/streams/pull/655#pullrequestreview-17680963