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

tyoshino commented on this pull request.



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

We could add asserts to code that looks at this so that nobody would use it before it gets initialized.

Actually, ... WritableStreamUpdateBackpressure was accidentally working correctly since it doesn't enter the code to modify readyPromise as there's no writer during construction.

But currently, it's clear that the backpressure signal is initialized in the constructor. I think it's ok to get this initialized as false.

Done

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

Also, I've fixed index.bs as the initialization code for backpressure was missing.

> -  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') {

I'd like to limit dependency to the stream's variables from conditions in the subroutines. But this should be fine. Done. Regarding putting the pendingAbortRequest into WritableStreamRejectPromisesInReactionToError, let me skip it for this reason.

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

Made some dedupe.

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

I've factored out the code to handle the errored case.

I don't want to call WritableStreamRejectPromisesInReactionToError() in close() handling code just because it's harmless. It's weird to have the logic looks for stuff remaining in `[[writeRequests]]` and `[[closeRequest]]` after finishing close() processing.

Sometimes it helps to have all-in-one helper to take care of various situations. But for this case, I feel like that we shouldn't. Increasing call depth together with dependency from leaf function calls would sometimes lead to smaller but harder-to-read code.

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

I struggled to have all the code look at the right thing (variables before/after update) when fixing the order of promise rejection/resolution. Saving old variables to `oldBlah` would also help but mess the code too. So, I've just let all the helpers take the new value directly than depending on that the caller makes sure to update the corresponding variables (e.g. `stream._storedError = reason`) before calling the helpers.

One benefit of letting this helper depend on `[[storedError]]` helps readers understand the invariants, i.e. the pending requests get rejected with the same exception as one in `[[storedError]]`. I understand that. So, there's trade-off.

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

Given that the current code updates `[[state]]` and `[[storedError]]` first, I can live with reverting this to depend on `[[storedError]]`, but still prefer having this take the new value directly.

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

I agree that it looks redundant to have the two separate slots. As you guessed, I chose this way for parallelism with `[[inFlightWriteRequest]]`.

We could also consolidate the inFlight indication for close and write into one variable.

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