Re: [whatwg/streams] Factor out condition to check pending operation (#672)

domenic commented on this pull request.



>  
     // The promise that was returned from writer.close(). Stored here because it may be fulfilled after the writer
     // has been detached.
-    this._pendingCloseRequest = undefined;
+    this._closeRequest = undefined;
+
+    // Close request is removed from _closeRequest when close() is called on the underlying sink. This prevents it
+    // from being erroneously rejected on error. If a close() call is in-flight, the request is stored here.
+    this._inFlightCloseRequest = undefined;

It seems like it'd be possible for implementations to shrink the object by re-using [[closeRequest]] and then adding an extra bit to the [[state]] + [[backpressure]] field. Do you think it would help the spec to use a boolean instead of a separate slot? My guess is that it's better as a separate slot (to parallel [[inFlightWriteRequest]]) but I wanted to check with you.

>  
     // The promise that was returned from writer.abort(). This may also be fulfilled after the writer has detached.
     this._pendingAbortRequest = undefined;
 
+    // The backpressure signal set by the controller.
+    this._backpressure = undefined;

I think this should be initialized to false? You don't seem to making use of it being undefined instead of false (all comparisons are === true) so keeping it always a boolean seems good to me.

>      writer._closedPromise.catch(() => {});
   }
 }
 
-function WritableStreamRejectPromisesInReactionToError(stream) {
+function WritableStreamRejectPromisesInReactionToError(stream, reason) {

As far as I can tell reason === storedError is always true. Why did you change it to be an argument?

> -  let wasAborted = false;
-  if (stream._pendingAbortRequest !== undefined) {
-    wasAborted = true;
-  }
-
-  let readyPromiseIsPending = false;
-  if (state === 'writable' && wasAborted === false &&
-      WritableStreamDefaultControllerGetBackpressure(stream._writableStreamController) === true) {
-    readyPromiseIsPending = true;
-  }
-
-  if (wasAborted === true) {
-    stream._pendingAbortRequest._reject(reason);
-    stream._pendingAbortRequest = undefined;
-  }
-
   if (state === 'errored') {

This code block appears above inside WritableStreamFinishInFlightWrite too. Maybe it should be factored out into a helper function (maybe even as part of WritableStreamRejectPromisesInReactionToError?)

>    }
 
-  WritableStreamRejectPromisesInReactionToError(stream);
+  stream._pendingAbortRequest._reject(reason);

This one is almost the same as the above two.

>  
   const state = stream._state;
 
-  let wasAborted = false;
-  if (stream._pendingAbortRequest !== undefined) {
-    wasAborted = true;
-  }
+  if (state === 'errored') {
+    if (stream._pendingAbortRequest !== undefined) {

Hmm another duplicated code block, but this time ending with WritableStreamRejectClosedPromiseIfAny instead of WritableStreamRejectPromisesInReactionToError. It feels like there is some abstraction missing...

-- 
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/672#pullrequestreview-25608670