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

isonmad commented on this pull request.

I still don't see how this improves code comprehensibility, honestly.

> @@ -97,40 +105,51 @@ function TransformStreamErrorInternal(transformStream, e) {
   }
 }
 
-function TransformStreamReadyPromise(transformStream) {
-  assert(transformStream._backpressureChangePromise !== undefined);
+// Used for preventing the next write() call on TransformStreamSink until there
+// is no longer backpressure.
+function TransformStreamBackpressureGonePromise(transformStream) {

I don't see how this name change makes it clearer. ReadyPromise is analogous to the [[readyPromise]] that writablestream writers have.

>  
     this._writable = new WritableStream(sink, transformer.writableStrategy);
 
     assert(this._writableController !== undefined);
     assert(this._readableController !== undefined);
 
     const desiredSize = this._readableController.desiredSize;
+    // Tentatively set _backpressure based on desiredSize. When desiredSize is 0, it's possible that a pull() is made
+    // immediately after this because of pending read()s and fix _backpressure back to false.

There can't be pending reads because this is the constructor, there's nothing 'tentative' about the fact that there is or isn't backpressure when the stream is first constructed.

> @@ -363,14 +386,18 @@ module.exports = class TransformStream {
 
     this._readable = new ReadableStream(source, transformer.readableStrategy);
 
-    const sink = new TransformStreamSink(this, startPromise);
+    const sink = new TransformStreamSink(this, startPromise.then(() => {
+      return TransformStreamBackpressureGonePromise(this);
+    }));

I don't see the point of doing this in the constructor instead of inside sink.start. It's conceptually something that .start does, and it's symmetrical with what source.start does. It's not really any clearer this way.

>      return this._startPromise.then(() => {
-      if (transformStream._readableBackpressure === true) {
-        return true;
+      // Prevent the first pull() call until there is backpressure.
+
+      assert(transformStream._backpressureChangePromise !== undefined,
+             '_backpressureChangePromise should have been initialized');

This added assert is good.

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