Re: [whatwg/streams] Refactor TransformStream's backpressure handling (#571)

ricea commented on this pull request.



>  
-  if (transformStream._readableBackpressure !== backpressure) {
-    TransformStreamSetBackpressure(transformStream, backpressure);
+  if (maybeBackpressure === true && transformStream._backpressure === false) {
+    // This allows pull() again. When desiredSize is 0, it's possible that a pull() is made immediately (but

How about "will happen immediately" rather than "is made immediately"?

>  
-  if (transformStream._readableBackpressure !== backpressure) {
-    TransformStreamSetBackpressure(transformStream, backpressure);
+  if (maybeBackpressure === true && transformStream._backpressure === false) {
+    // This allows pull() again. When desiredSize is 0, it's possible that a pull() is made immediately (but
+    // asynchronously) after this because of pending read()s and fix _backpressure back to false.

s/fix/set/ ?

>  
-  if (transformStream._readableBackpressure !== backpressure) {
-    TransformStreamSetBackpressure(transformStream, backpressure);
+  if (maybeBackpressure === true && transformStream._backpressure === false) {
+    // This allows pull() again. When desiredSize is 0, it's possible that a pull() is made immediately (but
+    // asynchronously) after this because of pending read()s and fix _backpressure back to false.
+    //
+    // We don't have to worry about that such a pull() may happen synchronously to the enqueue(), and therefore is not

Sorry, I don't understand what "synchronously to" means here. Maybe you could explain the dangerous case as a series of steps, and then explain why it never happens?

>  
   return transformStream._backpressureChangePromise;
 }
 
 function TransformStreamSetBackpressure(transformStream, backpressure) {
   // console.log(`TransformStreamSetBackpressure(${backpressure})`);
 
-  assert(transformStream._readableBackpressure !== backpressure);
+  // Passes also when called for initialization.

How about "during construction" instead of "for initialization"?

>  
   if (transformStream._backpressureChangePromise !== undefined) {
+    // The fulfillment value is just for the assertion.

"the assertion" is ambiguous because it's not clear which assertion is being referred to. How about "just for a sanity check"?

-- 
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/571#pullrequestreview-6231766