stream: finished should error on errored stream.

Calling finished before or after a stream has errored or closed should end up with the same behavior.
nodejs · ronag · Jul 2, 2021 · Jul 2, 2021 · Jul 2, 2021 · Jul 2, 2021
commit c6fb6196211e025212b0d9f6b5b99771e5dc801f
diff --git a/lib/_http_client.js b/lib/_http_client.js
@@ -806,6 +806,9 @@ function onSocketNT(req, socket, err) {
         socket.emit('free');
       } else {
         finished(socket.destroy(err || req[kError]), (er) => {
+          if (er.code === 'ERR_STREAM_PREMATURE_CLOSE') {
+            er = null;
+          }
           _destroy(req, er || err);
         });
         return;

diff --git a/lib/_http_incoming.js b/lib/_http_incoming.js
@@ -188,6 +188,9 @@ IncomingMessage.prototype._destroy = function _destroy(err, cb) {
   if (this.socket && !this.socket.destroyed && this.aborted) {
     this.socket.destroy(err);
     const cleanup = finished(this.socket, (e) => {
+      if (e.code === 'ERR_STREAM_PREMATURE_CLOSE') {
+        e = null;
+      }
       cleanup();
       process.nextTick(onError, this, e || err, cb);
     });

diff --git a/lib/internal/streams/end-of-stream.js b/lib/internal/streams/end-of-stream.js
@@ -128,7 +128,17 @@ function eos(stream, options, callback) {
     callback.call(stream, err);
   };
 
+  let closed = false;
+
   const onclose = () => {
+    closed = true;
+
+    const errored = wState?.errored || rState?.errored;
+
+    if (errored && typeof errored !== 'boolean') {
+      return callback.call(stream, errored);
+    }
+
     if (readable && !readableEnded) {
       if (!isReadableEnded(stream))
         return callback.call(stream,
@@ -139,6 +149,7 @@ function eos(stream, options, callback) {
         return callback.call(stream,
                              new ERR_STREAM_PREMATURE_CLOSE());
     }
+
     callback.call(stream);
   };
 
@@ -168,29 +179,29 @@ function eos(stream, options, callback) {
   if (options.error !== false) stream.on('error', onerror);
   stream.on('close', onclose);
 
-  // _closed is for OutgoingMessage which is not a proper Writable.
-  const closed = (!wState && !rState && stream._closed === true) || (
-    (wState && wState.closed) ||
-    (rState && rState.closed) ||
-    (wState && wState.errorEmitted) ||
-    (rState && rState.errorEmitted) ||
-    (rState && stream.req && stream.aborted) ||
-    (
-      (!writable || (wState && wState.finished)) &&
-      (!readable || (rState && rState.endEmitted))
-    )
-  );
-
-  if (closed) {
-    // TODO(ronag): Re-throw error if errorEmitted?
-    // TODO(ronag): Throw premature close as if finished was called?
-    // before being closed? i.e. if closed but not errored, ended or finished.
-    // TODO(ronag): Throw some kind of error? Does it make sense
-    // to call finished() on a "finished" stream?
-    // TODO(ronag): willEmitClose?
-    process.nextTick(() => {
-      callback();
-    });
+  if (closed || wState?.closed || rState?.closed) {
+    process.nextTick(onclose);
+  } else if (wState?.errorEmitted || rState?.errorEmitted) {
+    if (!willEmitClose) {
+      process.nextTick(onclose);
+    }
+  } else if (
+    !readable &&
+    (!willEmitClose || stream.readable) &&
+    writableFinished
+  ) {
+    process.nextTick(onclose);
+  } else if (
+    !writable &&
+    (!willEmitClose || stream.writable) &&
+    readableEnded
+  ) {
+    process.nextTick(onclose);
+  } else if (!wState && !rState && stream._closed === true) {
+    // _closed is for OutgoingMessage which is not a proper Writable.
+    process.nextTick(onclose);
+  } else if ((rState && stream.req && stream.aborted)) {
+    process.nextTick(onclose);
   }
 
   const cleanup = () => {

diff --git a/test/parallel/test-stream-finished.js b/test/parallel/test-stream-finished.js
@@ -608,3 +608,26 @@ testClosed((opts) => new Writable({ write() {}, ...opts }));
     assert.strictEqual(closed, true);
   }));
 }
+
+{
+  const w = new Writable();
+  const _err = new Error();
+  w.destroy(_err);
+  finished(w, common.mustCall((err) => {
+    assert.strictEqual(_err, err);
+    finished(w, common.mustCall((err) => {
+      assert.strictEqual(_err, err);
+    }));
+  }));
+}
+
+{
+  const w = new Writable();
+  w.destroy();
+  finished(w, common.mustCall((err) => {
+    assert.strictEqual(err.code, 'ERR_STREAM_PREMATURE_CLOSE');
+    finished(w, common.mustCall((err) => {
+      assert.strictEqual(err.code, 'ERR_STREAM_PREMATURE_CLOSE');
+    }));
+  }));
+}