From 97f8e1df64de273064fcb3602f51c1f37b4abe8d Mon Sep 17 00:00:00 2001
From: Robert Nagy <ronagy@icloud.com>
Date: Sun, 28 Jun 2020 18:43:59 +0200
Subject: [PATCH 1/8] stream: save error in state

Useful for future PR's to resolve situations where e.g. finished()
is invoked on an already errored streams.
---
 lib/_stream_readable.js                       |  2 +-
 lib/_stream_writable.js                       | 10 ++++---
 lib/internal/streams/destroy.js               | 28 +++++++++----------
 test/parallel/test-stream-readable-destroy.js | 14 +++++-----
 test/parallel/test-stream-writable-destroy.js |  9 +++---
 5 files changed, 33 insertions(+), 30 deletions(-)

diff --git a/lib/_stream_readable.js b/lib/_stream_readable.js
index d33d53de2b23a4..8ddb9562a41af2 100644
--- a/lib/_stream_readable.js
+++ b/lib/_stream_readable.js
@@ -155,7 +155,7 @@ function ReadableState(options, stream, isDuplex) {
   // _read calls, 'data' or 'readable' events should occur. This is needed
   // since when autoDestroy is disabled we need a way to tell whether the
   // stream has failed.
-  this.errored = false;
+  this.errored = null;
 
   // Indicates whether the stream has finished destroying.
   this.closed = false;
diff --git a/lib/_stream_writable.js b/lib/_stream_writable.js
index 916e9b87d9c17a..b63c8f89aec544 100644
--- a/lib/_stream_writable.js
+++ b/lib/_stream_writable.js
@@ -177,7 +177,7 @@ function WritableState(options, stream, isDuplex) {
   // Indicates whether the stream has errored. When true all write() calls
   // should return false. This is needed since when autoDestroy
   // is disabled we need a way to tell whether the stream has failed.
-  this.errored = false;
+  this.errored = null;
 
   // Indicates whether the stream has finished destroying.
   this.closed = false;
@@ -432,12 +432,14 @@ function onwrite(stream, er) {
   state.writelen = 0;
 
   if (er) {
-    state.errored = true;
+    if (!state.errored) {
+      state.errored = er;
+    }
 
     // In case of duplex streams we need to notify the readable side of the
     // error.
-    if (stream._readableState) {
-      stream._readableState.errored = true;
+    if (stream._readableState && !stream._readableState.errored) {
+      stream._readableState.errored = er;
     }
 
     if (sync) {
diff --git a/lib/internal/streams/destroy.js b/lib/internal/streams/destroy.js
index 58e3f5199445ed..2e8c8afd88cda4 100644
--- a/lib/internal/streams/destroy.js
+++ b/lib/internal/streams/destroy.js
@@ -25,11 +25,11 @@ function destroy(err, cb) {
   }
 
   if (err) {
-    if (w) {
-      w.errored = true;
+    if (w && !w.errored) {
+      w.errored = err;
     }
-    if (r) {
-      r.errored = true;
+    if (r && !r.errored) {
+      r.errored = err;
     }
   }
 
@@ -61,11 +61,11 @@ function _destroy(self, err, cb) {
     const w = self._writableState;
 
     if (err) {
-      if (w) {
-        w.errored = true;
+      if (w && !w.errored) {
+        w.errored = err;
       }
-      if (r) {
-        r.errored = true;
+      if (r && !r.errored) {
+        r.errored = err;
       }
     }
 
@@ -136,7 +136,7 @@ function undestroy() {
     r.closed = false;
     r.closeEmitted = false;
     r.destroyed = false;
-    r.errored = false;
+    r.errored = null;
     r.errorEmitted = false;
     r.reading = false;
     r.ended = false;
@@ -148,7 +148,7 @@ function undestroy() {
     w.destroyed = false;
     w.closed = false;
     w.closeEmitted = false;
-    w.errored = false;
+    w.errored = null;
     w.errorEmitted = false;
     w.ended = false;
     w.ending = false;
@@ -175,11 +175,11 @@ function errorOrDestroy(stream, err, sync) {
   if ((r && r.autoDestroy) || (w && w.autoDestroy))
     stream.destroy(err);
   else if (err) {
-    if (w) {
-      w.errored = true;
+    if (w && w.errored) {
+      w.errored = err;
     }
-    if (r) {
-      r.errored = true;
+    if (r && r.errored) {
+      r.errored = err;
     }
     if (sync) {
       process.nextTick(emitErrorNT, stream, err);
diff --git a/test/parallel/test-stream-readable-destroy.js b/test/parallel/test-stream-readable-destroy.js
index 3d1ac8c92f9bd3..8d4c05a34b3647 100644
--- a/test/parallel/test-stream-readable-destroy.js
+++ b/test/parallel/test-stream-readable-destroy.js
@@ -134,13 +134,13 @@ const assert = require('assert');
   read.on('error', common.mustCall((err) => {
     assert.strictEqual(ticked, true);
     assert.strictEqual(read._readableState.errorEmitted, true);
-    assert.strictEqual(read._readableState.errored, true);
+    assert.strictEqual(read._readableState.errored, expected);
     assert.strictEqual(err, expected);
   }));
 
   read.destroy();
   assert.strictEqual(read._readableState.errorEmitted, false);
-  assert.strictEqual(read._readableState.errored, true);
+  assert.strictEqual(read._readableState.errored, expected);
   assert.strictEqual(read.destroyed, true);
   ticked = true;
 }
@@ -190,15 +190,15 @@ const assert = require('assert');
     assert.strictEqual(err, expected);
   }));
 
-  assert.strictEqual(read._readableState.errored, false);
+  assert.strictEqual(read._readableState.errored, null);
   assert.strictEqual(read._readableState.errorEmitted, false);
 
   read.destroy(expected, common.mustCall(function(err) {
-    assert.strictEqual(read._readableState.errored, true);
+    assert.strictEqual(read._readableState.errored, expected);
     assert.strictEqual(err, expected);
   }));
   assert.strictEqual(read._readableState.errorEmitted, false);
-  assert.strictEqual(read._readableState.errored, true);
+  assert.strictEqual(read._readableState.errored, expected);
   ticked = true;
 }
 
@@ -223,14 +223,14 @@ const assert = require('assert');
 
   readable.destroy();
   assert.strictEqual(readable.destroyed, true);
-  assert.strictEqual(readable._readableState.errored, false);
+  assert.strictEqual(readable._readableState.errored, null);
   assert.strictEqual(readable._readableState.errorEmitted, false);
 
   // Test case where `readable.destroy()` is called again with an error before
   // the `_destroy()` callback is called.
   readable.destroy(new Error('kaboom 2'));
   assert.strictEqual(readable._readableState.errorEmitted, false);
-  assert.strictEqual(readable._readableState.errored, false);
+  assert.strictEqual(readable._readableState.errored, null);
 
   ticked = true;
 }
diff --git a/test/parallel/test-stream-writable-destroy.js b/test/parallel/test-stream-writable-destroy.js
index 706847a8582d0c..7036f60d00887e 100644
--- a/test/parallel/test-stream-writable-destroy.js
+++ b/test/parallel/test-stream-writable-destroy.js
@@ -167,9 +167,10 @@ const assert = require('assert');
     assert.strictEqual(write._writableState.errorEmitted, true);
   }));
 
-  write.destroy(new Error('kaboom 1'));
+  const expected = new Error('kaboom 1');
+  write.destroy(expected);
   write.destroy(new Error('kaboom 2'));
-  assert.strictEqual(write._writableState.errored, true);
+  assert.strictEqual(write._writableState.errored, expected);
   assert.strictEqual(write._writableState.errorEmitted, false);
   assert.strictEqual(write.destroyed, true);
   ticked = true;
@@ -200,14 +201,14 @@ const assert = require('assert');
 
   writable.destroy();
   assert.strictEqual(writable.destroyed, true);
-  assert.strictEqual(writable._writableState.errored, false);
+  assert.strictEqual(writable._writableState.errored, null);
   assert.strictEqual(writable._writableState.errorEmitted, false);
 
   // Test case where `writable.destroy()` is called again with an error before
   // the `_destroy()` callback is called.
   writable.destroy(new Error('kaboom 2'));
   assert.strictEqual(writable._writableState.errorEmitted, false);
-  assert.strictEqual(writable._writableState.errored, false);
+  assert.strictEqual(writable._writableState.errored, null);
 
   ticked = true;
 }

From 2043f7cbbeeaf33790b7a54dfde1aeef47da9840 Mon Sep 17 00:00:00 2001
From: Robert Nagy <ronagy@icloud.com>
Date: Mon, 29 Jun 2020 13:25:49 +0200
Subject: [PATCH 2/8] fixup: tls gc

---
 lib/_tls_wrap.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js
index 02fd7b002651c3..ca689574bfcf72 100644
--- a/lib/_tls_wrap.js
+++ b/lib/_tls_wrap.js
@@ -625,7 +625,7 @@ function defineHandleReading(socket, handle) {
 
 function onSocketCloseDestroySSL() {
   // Make sure we are not doing it on OpenSSL's stack
-  setImmediate(destroySSL, this);
+  setImmediate(destroySSL.bind(null, this));
   this[kRes] = null;
 }
 

From ceb121521bc7637e772c0b9c523403e0a3d2c169 Mon Sep 17 00:00:00 2001
From: Robert Nagy <ronagy@icloud.com>
Date: Mon, 29 Jun 2020 20:20:41 +0200
Subject: [PATCH 3/8] Update lib/_tls_wrap.js

Co-authored-by: Anna Henningsen <github@addaleax.net>
---
 lib/_tls_wrap.js | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js
index ca689574bfcf72..606d4968fa3935 100644
--- a/lib/_tls_wrap.js
+++ b/lib/_tls_wrap.js
@@ -625,6 +625,9 @@ function defineHandleReading(socket, handle) {
 
 function onSocketCloseDestroySSL() {
   // Make sure we are not doing it on OpenSSL's stack
+  // This should not be using .bind(). However, doing so appears necessary
+  // to work around a garbage collection bug:
+  // Refs: https://github.com/nodejs/node/pull/34103#issuecomment-651040237
   setImmediate(destroySSL.bind(null, this));
   this[kRes] = null;
 }

From c7d5f90ba3f052a489e3938db9d860c32386216c Mon Sep 17 00:00:00 2001
From: Robert Nagy <ronagy@icloud.com>
Date: Tue, 30 Jun 2020 20:52:45 +0200
Subject: [PATCH 4/8] fixup

---
 lib/internal/streams/destroy.js | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/internal/streams/destroy.js b/lib/internal/streams/destroy.js
index 2e8c8afd88cda4..575b12f9c18b9a 100644
--- a/lib/internal/streams/destroy.js
+++ b/lib/internal/streams/destroy.js
@@ -175,10 +175,10 @@ function errorOrDestroy(stream, err, sync) {
   if ((r && r.autoDestroy) || (w && w.autoDestroy))
     stream.destroy(err);
   else if (err) {
-    if (w && w.errored) {
+    if (w && !w.errored) {
       w.errored = err;
     }
-    if (r && r.errored) {
+    if (r && !r.errored) {
       r.errored = err;
     }
     if (sync) {

From 2c490cd5b088333032ce1c059d3abaad77391cc2 Mon Sep 17 00:00:00 2001
From: Robert Nagy <ronagy@icloud.com>
Date: Tue, 30 Jun 2020 22:01:26 +0200
Subject: [PATCH 5/8] Revert "fixup: tls gc"

This reverts commit 7bf789b9d76c832ca8cd8e664163304d8f4006c2.
---
 lib/_tls_wrap.js | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js
index 606d4968fa3935..02fd7b002651c3 100644
--- a/lib/_tls_wrap.js
+++ b/lib/_tls_wrap.js
@@ -625,10 +625,7 @@ function defineHandleReading(socket, handle) {
 
 function onSocketCloseDestroySSL() {
   // Make sure we are not doing it on OpenSSL's stack
-  // This should not be using .bind(). However, doing so appears necessary
-  // to work around a garbage collection bug:
-  // Refs: https://github.com/nodejs/node/pull/34103#issuecomment-651040237
-  setImmediate(destroySSL.bind(null, this));
+  setImmediate(destroySSL, this);
   this[kRes] = null;
 }
 

From 13fb73b9bc98ee485c1884b7229f931b267c6f1f Mon Sep 17 00:00:00 2001
From: Robert Nagy <ronagy@icloud.com>
Date: Tue, 30 Jun 2020 22:03:42 +0200
Subject: [PATCH 6/8] fixup: err stack leak

---
 lib/_stream_writable.js         | 3 +++
 lib/internal/streams/destroy.js | 9 +++++++++
 2 files changed, 12 insertions(+)

diff --git a/lib/_stream_writable.js b/lib/_stream_writable.js
index b63c8f89aec544..c3681106322c52 100644
--- a/lib/_stream_writable.js
+++ b/lib/_stream_writable.js
@@ -432,6 +432,9 @@ function onwrite(stream, er) {
   state.writelen = 0;
 
   if (er) {
+    // Avoid V8 leak, https://github.com/nodejs/node/pull/34103#issuecomment-652002364
+    er.stack;
+
     if (!state.errored) {
       state.errored = er;
     }
diff --git a/lib/internal/streams/destroy.js b/lib/internal/streams/destroy.js
index 575b12f9c18b9a..a2a91245ca46ea 100644
--- a/lib/internal/streams/destroy.js
+++ b/lib/internal/streams/destroy.js
@@ -25,6 +25,9 @@ function destroy(err, cb) {
   }
 
   if (err) {
+    // Avoid V8 leak, https://github.com/nodejs/node/pull/34103#issuecomment-652002364
+    err.stack;
+
     if (w && !w.errored) {
       w.errored = err;
     }
@@ -61,6 +64,9 @@ function _destroy(self, err, cb) {
     const w = self._writableState;
 
     if (err) {
+      // Avoid V8 leak, https://github.com/nodejs/node/pull/34103#issuecomment-652002364
+      err.stack;
+
       if (w && !w.errored) {
         w.errored = err;
       }
@@ -175,6 +181,9 @@ function errorOrDestroy(stream, err, sync) {
   if ((r && r.autoDestroy) || (w && w.autoDestroy))
     stream.destroy(err);
   else if (err) {
+    // Avoid V8 leak, https://github.com/nodejs/node/pull/34103#issuecomment-652002364
+    err.stack;
+
     if (w && !w.errored) {
       w.errored = err;
     }

From e0404de6f2bd933efa3aa0d3d9c0e64272035e8f Mon Sep 17 00:00:00 2001
From: Robert Nagy <ronagy@icloud.com>
Date: Tue, 30 Jun 2020 22:10:52 +0200
Subject: [PATCH 7/8] fixup: add tests

---
 test/parallel/test-stream-readable-destroy.js | 15 +++++++++++++++
 test/parallel/test-stream-writable-destroy.js | 15 +++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/test/parallel/test-stream-readable-destroy.js b/test/parallel/test-stream-readable-destroy.js
index 8d4c05a34b3647..8ab78ec8ccec35 100644
--- a/test/parallel/test-stream-readable-destroy.js
+++ b/test/parallel/test-stream-readable-destroy.js
@@ -253,3 +253,18 @@ const assert = require('assert');
   assert.strictEqual(read.destroyed, true);
   read.read();
 }
+
+{
+  const read = new Readable({
+    autoDestroy: false,
+    read() {
+      this.push(null);
+      this.push('asd');
+    }
+  });
+
+  read.on('error', common.mustCall(() => {
+    assert(read._readableState.errored);
+  }));
+  read.resume();
+}
diff --git a/test/parallel/test-stream-writable-destroy.js b/test/parallel/test-stream-writable-destroy.js
index 7036f60d00887e..9f6136923ee176 100644
--- a/test/parallel/test-stream-writable-destroy.js
+++ b/test/parallel/test-stream-writable-destroy.js
@@ -402,3 +402,18 @@ const assert = require('assert');
   }));
   write.destroy();
 }
+
+{
+  const write = new Writable({
+    autoDestroy: false,
+    write(chunk, enc, cb) {
+      cb();
+      cb();
+    }
+  });
+
+  write.on('error', common.mustCall(() => {
+    assert(write._writableState.errored);
+  }));
+  write.write('asd');
+}

From 1953d03064f56b88888321aa69c280f426bc616d Mon Sep 17 00:00:00 2001
From: Robert Nagy <ronagy@icloud.com>
Date: Tue, 30 Jun 2020 23:57:44 +0200
Subject: [PATCH 8/8] fixup

---
 test/parallel/test-stream2-readable-wrap-error.js | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/test/parallel/test-stream2-readable-wrap-error.js b/test/parallel/test-stream2-readable-wrap-error.js
index b56b9bc41c7527..ac1f64f69810f0 100644
--- a/test/parallel/test-stream2-readable-wrap-error.js
+++ b/test/parallel/test-stream2-readable-wrap-error.js
@@ -10,23 +10,25 @@ oldStream.pause = () => {};
 oldStream.resume = () => {};
 
 {
+  const err = new Error();
   const r = new Readable({ autoDestroy: true })
     .wrap(oldStream)
     .on('error', common.mustCall(() => {
       assert.strictEqual(r._readableState.errorEmitted, true);
-      assert.strictEqual(r._readableState.errored, true);
+      assert.strictEqual(r._readableState.errored, err);
       assert.strictEqual(r.destroyed, true);
     }));
-  oldStream.emit('error', new Error());
+  oldStream.emit('error', err);
 }
 
 {
+  const err = new Error();
   const r = new Readable({ autoDestroy: false })
     .wrap(oldStream)
     .on('error', common.mustCall(() => {
       assert.strictEqual(r._readableState.errorEmitted, true);
-      assert.strictEqual(r._readableState.errored, true);
+      assert.strictEqual(r._readableState.errored, err);
       assert.strictEqual(r.destroyed, false);
     }));
-  oldStream.emit('error', new Error());
+  oldStream.emit('error', err);
 }