From 95152048c90d29d2dfe9681cfea31800f3908281 Mon Sep 17 00:00:00 2001
From: Robert Nagy <ronagy@icloud.com>
Date: Thu, 30 Apr 2020 20:29:35 +0200
Subject: [PATCH 1/6] http: emit 'error' on aborted server request

Server requests aka. IncomingMessage emits 'aborted'
instead of 'error' which causes confusion when
the object is used as a regular stream, i.e. if
functions working on streams are passed a
server request object they might not work properly
unless they take this into account.

Refs: https://github.com/nodejs/web-server-frameworks/issues/41
---
 lib/_http_server.js                | 10 ++++-
 test/parallel/test-http-aborted.js | 61 ++++++++++++++++++++++--------
 2 files changed, 54 insertions(+), 17 deletions(-)

diff --git a/lib/_http_server.js b/lib/_http_server.js
index c8b27dcd42cb5b..f52ec2ffa2008c 100644
--- a/lib/_http_server.js
+++ b/lib/_http_server.js
@@ -56,12 +56,16 @@ const {
   getOrSetAsyncId
 } = require('internal/async_hooks');
 const { IncomingMessage } = require('_http_incoming');
+const {
+  connResetException,
+  codes
+} = require('internal/errors');
 const {
   ERR_HTTP_HEADERS_SENT,
   ERR_HTTP_INVALID_STATUS_CODE,
   ERR_INVALID_ARG_TYPE,
   ERR_INVALID_CHAR
-} = require('internal/errors').codes;
+} = codes;
 const { validateInteger } = require('internal/validators');
 const Buffer = require('buffer').Buffer;
 const {
@@ -533,8 +537,12 @@ function socketOnClose(socket, state) {
 function abortIncoming(incoming) {
   while (incoming.length) {
     const req = incoming.shift();
+    // TODO(ronag): req.destroy(err)
     req.aborted = true;
     req.emit('aborted');
+    if (req.listenerCount('error') > 0) {
+      req.emit('error', connResetException('aborted'));
+    }
     req.emit('close');
   }
   // Abort socket._httpMessage ?
diff --git a/test/parallel/test-http-aborted.js b/test/parallel/test-http-aborted.js
index c3d7e4641f4501..44ee106b18df06 100644
--- a/test/parallel/test-http-aborted.js
+++ b/test/parallel/test-http-aborted.js
@@ -4,23 +4,52 @@ const common = require('../common');
 const http = require('http');
 const assert = require('assert');
 
-const server = http.createServer(common.mustCall(function(req, res) {
-  req.on('aborted', common.mustCall(function() {
-    assert.strictEqual(this.aborted, true);
-    server.close();
+{
+  const server = http.createServer(common.mustCall(function(req, res) {
+    req.on('aborted', common.mustCall(function() {
+      assert.strictEqual(this.aborted, true);
+    }));
+    req.on('error', common.mustCall(function(err) {
+      assert.strictEqual(err.code, 'ECONNRESET');
+      server.close();
+    }));
+    assert.strictEqual(req.aborted, false);
+    res.write('hello');
+  }));
+
+  server.listen(0, common.mustCall(() => {
+    const req = http.get({
+      port: server.address().port,
+      headers: { connection: 'keep-alive' }
+    }, common.mustCall((res) => {
+      res.on('aborted', common.mustCall(() => {
+        assert.strictEqual(res.aborted, true);
+      }));
+      req.abort();
+    }));
+  }));
+}
+
+{
+  // Don't crash if no 'error' handler on server request.
+
+  const server = http.createServer(common.mustCall(function(req, res) {
+    req.on('aborted', common.mustCall(function() {
+      assert.strictEqual(this.aborted, true);
+    }));
+    assert.strictEqual(req.aborted, false);
+    res.write('hello');
   }));
-  assert.strictEqual(req.aborted, false);
-  res.write('hello');
-}));
 
-server.listen(0, common.mustCall(() => {
-  const req = http.get({
-    port: server.address().port,
-    headers: { connection: 'keep-alive' }
-  }, common.mustCall((res) => {
-    res.on('aborted', common.mustCall(() => {
-      assert.strictEqual(res.aborted, true);
+  server.listen(0, common.mustCall(() => {
+    const req = http.get({
+      port: server.address().port,
+      headers: { connection: 'keep-alive' }
+    }, common.mustCall((res) => {
+      res.on('aborted', common.mustCall(() => {
+        assert.strictEqual(res.aborted, true);
+      }));
+      req.abort();
     }));
-    req.abort();
   }));
-}));
+}

From 4bcd762732ec52b9a79501eef0609adf14777208 Mon Sep 17 00:00:00 2001
From: Robert Nagy <ronagy@icloud.com>
Date: Thu, 30 Apr 2020 20:41:21 +0200
Subject: [PATCH 2/6] http: emit 'error' on aborted client response

Client responses aka. IncomingMessage emits 'aborted'
instead of 'error' which causes confusion when
the object is used as a regular stream, i.e. if
functions working on streams are passed a
client response object they might not work properly
unless they take this into account.

Refs: https://github.com/nodejs/web-server-frameworks/issues/41
Fixes: https://github.com/nodejs/node/issues/28172
---
 doc/api/http.md                               | 11 ++--
 lib/_http_client.js                           |  4 ++
 test/parallel/test-http-abort-client.js       |  4 ++
 test/parallel/test-http-aborted.js            |  4 ++
 .../test-http-client-aborted-event.js         | 54 ++++++++++++++-----
 .../test-http-client-spurious-aborted.js      |  5 +-
 ...http-outgoing-message-capture-rejection.js |  3 ++
 7 files changed, 67 insertions(+), 18 deletions(-)

diff --git a/doc/api/http.md b/doc/api/http.md
index 10d728a13d4523..3adccd1b127912 100644
--- a/doc/api/http.md
+++ b/doc/api/http.md
@@ -333,9 +333,8 @@ Until the data is consumed, the `'end'` event will not fire. Also, until
 the data is read it will consume memory that can eventually lead to a
 'process out of memory' error.
 
-Unlike the `request` object, if the response closes prematurely, the
-`response` object does not emit an `'error'` event but instead emits the
-`'aborted'` event.
+For backward compatibility, `res` will only emit `'error'` if there is an
+`'error'` listener registered.
 
 Node.js does not check whether Content-Length and the length of the
 body which has been transmitted are equal or not.
@@ -2404,6 +2403,8 @@ the following events will be emitted in the following order:
   * `'data'` any number of times, on the `res` object
 * (connection closed here)
 * `'aborted'` on the `res` object
+* `'error'` on the `res` object with an error with message
+  `'Error: socket hang up'` and code `'ECONNRESET'`.
 * `'close'`
 * `'close'` on the `res` object
 
@@ -2432,6 +2433,8 @@ events will be emitted in the following order:
   * `'data'` any number of times, on the `res` object
 * (`req.destroy()` called here)
 * `'aborted'` on the `res` object
+* `'error'` on the `res` object with an error with message
+  `'Error: socket hang up'` and code `'ECONNRESET'`.
 * `'close'`
 * `'close'` on the `res` object
 
@@ -2461,6 +2464,8 @@ events will be emitted in the following order:
 * (`req.abort()` called here)
 * `'abort'`
 * `'aborted'` on the `res` object
+* `'error'` on the `res` object with an error with message
+  `'Error: socket hang up'` and code `'ECONNRESET'`.
 * `'close'`
 * `'close'` on the `res` object
 
diff --git a/lib/_http_client.js b/lib/_http_client.js
index 73a6409c411cc1..886489dd939227 100644
--- a/lib/_http_client.js
+++ b/lib/_http_client.js
@@ -413,9 +413,13 @@ function socketCloseListener() {
   const res = req.res;
   if (res) {
     // Socket closed before we emitted 'end' below.
+    // TOOD(ronag): res.destroy(err)
     if (!res.complete) {
       res.aborted = true;
       res.emit('aborted');
+      if (res.listenerCount('error') > 0) {
+        res.emit('error', connResetException('aborted'));
+      }
     }
     req.emit('close');
     if (!res.aborted && res.readable) {
diff --git a/test/parallel/test-http-abort-client.js b/test/parallel/test-http-abort-client.js
index 608d4dc7607853..3664237c53c6e0 100644
--- a/test/parallel/test-http-abort-client.js
+++ b/test/parallel/test-http-abort-client.js
@@ -21,6 +21,7 @@
 
 'use strict';
 const common = require('../common');
+const assert = require('assert');
 const http = require('http');
 
 let serverRes;
@@ -41,6 +42,9 @@ server.listen(0, common.mustCall(() => {
     res.resume();
     res.on('end', common.mustNotCall());
     res.on('aborted', common.mustCall());
+    res.on('error', common.mustCall((err) => {
+      assert.strictEqual(err.code, 'ECONNRESET');
+    }));
     res.on('close', common.mustCall());
     res.socket.on('close', common.mustCall());
   }));
diff --git a/test/parallel/test-http-aborted.js b/test/parallel/test-http-aborted.js
index 44ee106b18df06..ff45469e45e3c2 100644
--- a/test/parallel/test-http-aborted.js
+++ b/test/parallel/test-http-aborted.js
@@ -25,6 +25,9 @@ const assert = require('assert');
       res.on('aborted', common.mustCall(() => {
         assert.strictEqual(res.aborted, true);
       }));
+      res.on('error', common.mustCall((err) => {
+        assert.strictEqual(err.code, 'ECONNRESET');
+      }));
       req.abort();
     }));
   }));
@@ -36,6 +39,7 @@ const assert = require('assert');
   const server = http.createServer(common.mustCall(function(req, res) {
     req.on('aborted', common.mustCall(function() {
       assert.strictEqual(this.aborted, true);
+      server.close();
     }));
     assert.strictEqual(req.aborted, false);
     res.write('hello');
diff --git a/test/parallel/test-http-client-aborted-event.js b/test/parallel/test-http-client-aborted-event.js
index 1e7feb7d5860f6..f73672f5db69ec 100644
--- a/test/parallel/test-http-client-aborted-event.js
+++ b/test/parallel/test-http-client-aborted-event.js
@@ -1,20 +1,46 @@
 'use strict';
 const common = require('../common');
 const http = require('http');
+const assert = require('assert');
 
-let serverRes;
-const server = http.Server(function(req, res) {
-  res.write('Part of my res.');
-  serverRes = res;
-});
+{
+  let serverRes;
+  const server = http.Server(function(req, res) {
+    res.write('Part of my res.');
+    serverRes = res;
+  });
 
-server.listen(0, common.mustCall(function() {
-  http.get({
-    port: this.address().port,
-    headers: { connection: 'keep-alive' }
-  }, common.mustCall(function(res) {
-    server.close();
-    serverRes.destroy();
-    res.on('aborted', common.mustCall());
+  server.listen(0, common.mustCall(function() {
+    http.get({
+      port: this.address().port,
+      headers: { connection: 'keep-alive' }
+    }, common.mustCall(function(res) {
+      server.close();
+      serverRes.destroy();
+      res.on('aborted', common.mustCall());
+      res.on('error', common.mustCall((err) => {
+        assert.strictEqual(err.code, 'ECONNRESET');
+      }));
+    }));
   }));
-}));
+}
+
+{
+  // Don't crash of no 'error' handler.
+  let serverRes;
+  const server = http.Server(function(req, res) {
+    res.write('Part of my res.');
+    serverRes = res;
+  });
+
+  server.listen(0, common.mustCall(function() {
+    http.get({
+      port: this.address().port,
+      headers: { connection: 'keep-alive' }
+    }, common.mustCall(function(res) {
+      server.close();
+      serverRes.destroy();
+      res.on('aborted', common.mustCall());
+    }));
+  }));
+}
diff --git a/test/parallel/test-http-client-spurious-aborted.js b/test/parallel/test-http-client-spurious-aborted.js
index 0cb2f471c2b792..7809c453a2ae18 100644
--- a/test/parallel/test-http-client-spurious-aborted.js
+++ b/test/parallel/test-http-client-spurious-aborted.js
@@ -60,15 +60,18 @@ function download() {
       res.on('end', common.mustCall(() => {
         reqCountdown.dec();
       }));
+      res.on('error', common.mustNotCall());
     } else {
       res.on('aborted', common.mustCall(() => {
         aborted = true;
         reqCountdown.dec();
         writable.end();
       }));
+      res.on('error', common.mustCall((err) => {
+        assert.strictEqual(err.code, 'ECONNRESET');
+      }));
     }
 
-    res.on('error', common.mustNotCall());
     writable.on('finish', () => {
       assert.strictEqual(aborted, abortRequest);
       finishCountdown.dec();
diff --git a/test/parallel/test-http-outgoing-message-capture-rejection.js b/test/parallel/test-http-outgoing-message-capture-rejection.js
index 5f667ea17ea156..fbe9f305b52bca 100644
--- a/test/parallel/test-http-outgoing-message-capture-rejection.js
+++ b/test/parallel/test-http-outgoing-message-capture-rejection.js
@@ -33,6 +33,9 @@ events.captureRejections = true;
 
     req.on('response', common.mustCall((res) => {
       res.on('aborted', common.mustCall());
+      res.on('error', common.mustCall((err) => {
+        assert.strictEqual(err.code, 'ECONNRESET');
+      }));
       res.resume();
       server.close();
     }));

From 0c54b5f33f8e4c567552b30c3b7f80a32f2dd371 Mon Sep 17 00:00:00 2001
From: Robert Nagy <ronagy@icloud.com>
Date: Fri, 1 May 2020 10:56:45 +0200
Subject: [PATCH 3/6] fixup! http: emit 'error' on aborted client response

---
 doc/api/http.md                    | 6 +++---
 test/parallel/test-http-aborted.js | 2 ++
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/doc/api/http.md b/doc/api/http.md
index 3adccd1b127912..21f477304fcdb2 100644
--- a/doc/api/http.md
+++ b/doc/api/http.md
@@ -2404,7 +2404,7 @@ the following events will be emitted in the following order:
 * (connection closed here)
 * `'aborted'` on the `res` object
 * `'error'` on the `res` object with an error with message
-  `'Error: socket hang up'` and code `'ECONNRESET'`.
+  `'Error: aborted'` and code `'ECONNRESET'`.
 * `'close'`
 * `'close'` on the `res` object
 
@@ -2434,7 +2434,7 @@ events will be emitted in the following order:
 * (`req.destroy()` called here)
 * `'aborted'` on the `res` object
 * `'error'` on the `res` object with an error with message
-  `'Error: socket hang up'` and code `'ECONNRESET'`.
+  `'Error: aborted'` and code `'ECONNRESET'`.
 * `'close'`
 * `'close'` on the `res` object
 
@@ -2465,7 +2465,7 @@ events will be emitted in the following order:
 * `'abort'`
 * `'aborted'` on the `res` object
 * `'error'` on the `res` object with an error with message
-  `'Error: socket hang up'` and code `'ECONNRESET'`.
+  `'Error: aborted'` and code `'ECONNRESET'`.
 * `'close'`
 * `'close'` on the `res` object
 
diff --git a/test/parallel/test-http-aborted.js b/test/parallel/test-http-aborted.js
index ff45469e45e3c2..f1a1e8b82bb94e 100644
--- a/test/parallel/test-http-aborted.js
+++ b/test/parallel/test-http-aborted.js
@@ -11,6 +11,7 @@ const assert = require('assert');
     }));
     req.on('error', common.mustCall(function(err) {
       assert.strictEqual(err.code, 'ECONNRESET');
+      assert.strictEqual(err.message, 'aborted');
       server.close();
     }));
     assert.strictEqual(req.aborted, false);
@@ -27,6 +28,7 @@ const assert = require('assert');
       }));
       res.on('error', common.mustCall((err) => {
         assert.strictEqual(err.code, 'ECONNRESET');
+        assert.strictEqual(err.message, 'aborted');
       }));
       req.abort();
     }));

From f7927246f71ae88879e462756ec0e2d606bb7a03 Mon Sep 17 00:00:00 2001
From: Robert Nagy <ronagy@icloud.com>
Date: Sun, 3 May 2020 18:34:08 +0200
Subject: [PATCH 4/6] Apply suggestions from code review

Co-authored-by: Ruben Bridgewater <ruben@bridgewater.de>
---
 test/parallel/test-http-abort-client.js                     | 4 ++--
 test/parallel/test-http-aborted.js                          | 6 +++---
 test/parallel/test-http-client-aborted-event.js             | 4 ++--
 test/parallel/test-http-client-spurious-aborted.js          | 4 ++--
 .../test-http-outgoing-message-capture-rejection.js         | 4 ++--
 5 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/test/parallel/test-http-abort-client.js b/test/parallel/test-http-abort-client.js
index 3664237c53c6e0..cddd8c883ff386 100644
--- a/test/parallel/test-http-abort-client.js
+++ b/test/parallel/test-http-abort-client.js
@@ -42,8 +42,8 @@ server.listen(0, common.mustCall(() => {
     res.resume();
     res.on('end', common.mustNotCall());
     res.on('aborted', common.mustCall());
-    res.on('error', common.mustCall((err) => {
-      assert.strictEqual(err.code, 'ECONNRESET');
+    res.on('error', common.expectsError({
+      code: 'ECONNRESET'
     }));
     res.on('close', common.mustCall());
     res.socket.on('close', common.mustCall());
diff --git a/test/parallel/test-http-aborted.js b/test/parallel/test-http-aborted.js
index f1a1e8b82bb94e..e22e7ca4cc330f 100644
--- a/test/parallel/test-http-aborted.js
+++ b/test/parallel/test-http-aborted.js
@@ -26,9 +26,9 @@ const assert = require('assert');
       res.on('aborted', common.mustCall(() => {
         assert.strictEqual(res.aborted, true);
       }));
-      res.on('error', common.mustCall((err) => {
-        assert.strictEqual(err.code, 'ECONNRESET');
-        assert.strictEqual(err.message, 'aborted');
+      res.on('error', common.expectsError({
+        code: 'ECONNRESET',
+        message: 'aborted'
       }));
       req.abort();
     }));
diff --git a/test/parallel/test-http-client-aborted-event.js b/test/parallel/test-http-client-aborted-event.js
index f73672f5db69ec..d666d413652145 100644
--- a/test/parallel/test-http-client-aborted-event.js
+++ b/test/parallel/test-http-client-aborted-event.js
@@ -18,8 +18,8 @@ const assert = require('assert');
       server.close();
       serverRes.destroy();
       res.on('aborted', common.mustCall());
-      res.on('error', common.mustCall((err) => {
-        assert.strictEqual(err.code, 'ECONNRESET');
+      res.on('error', common.expectsError({
+        code: 'ECONNRESET'
       }));
     }));
   }));
diff --git a/test/parallel/test-http-client-spurious-aborted.js b/test/parallel/test-http-client-spurious-aborted.js
index 7809c453a2ae18..850462dadc76f8 100644
--- a/test/parallel/test-http-client-spurious-aborted.js
+++ b/test/parallel/test-http-client-spurious-aborted.js
@@ -67,8 +67,8 @@ function download() {
         reqCountdown.dec();
         writable.end();
       }));
-      res.on('error', common.mustCall((err) => {
-        assert.strictEqual(err.code, 'ECONNRESET');
+      res.on('error', common.expectsError({
+        code: 'ECONNRESET'
       }));
     }
 
diff --git a/test/parallel/test-http-outgoing-message-capture-rejection.js b/test/parallel/test-http-outgoing-message-capture-rejection.js
index fbe9f305b52bca..9fe9bdb21331d8 100644
--- a/test/parallel/test-http-outgoing-message-capture-rejection.js
+++ b/test/parallel/test-http-outgoing-message-capture-rejection.js
@@ -33,8 +33,8 @@ events.captureRejections = true;
 
     req.on('response', common.mustCall((res) => {
       res.on('aborted', common.mustCall());
-      res.on('error', common.mustCall((err) => {
-        assert.strictEqual(err.code, 'ECONNRESET');
+      res.on('error', common.expectsError({
+        code: 'ECONNRESET'
       }));
       res.resume();
       server.close();

From 49baa70626ab1028d7f4182eaaba9725f322b86b Mon Sep 17 00:00:00 2001
From: Robert Nagy <ronagy@icloud.com>
Date: Wed, 6 May 2020 22:38:11 +0200
Subject: [PATCH 5/6] fixup

---
 test/parallel/test-http-abort-client.js       |  1 -
 .../test-http-client-aborted-event.js         |  1 -
 test/parallel/test-stream-readable-read.js    | 44 +++++++++++++++++++
 3 files changed, 44 insertions(+), 2 deletions(-)
 create mode 100644 test/parallel/test-stream-readable-read.js

diff --git a/test/parallel/test-http-abort-client.js b/test/parallel/test-http-abort-client.js
index cddd8c883ff386..8a4666df1c28c3 100644
--- a/test/parallel/test-http-abort-client.js
+++ b/test/parallel/test-http-abort-client.js
@@ -21,7 +21,6 @@
 
 'use strict';
 const common = require('../common');
-const assert = require('assert');
 const http = require('http');
 
 let serverRes;
diff --git a/test/parallel/test-http-client-aborted-event.js b/test/parallel/test-http-client-aborted-event.js
index d666d413652145..b1401187705acb 100644
--- a/test/parallel/test-http-client-aborted-event.js
+++ b/test/parallel/test-http-client-aborted-event.js
@@ -1,7 +1,6 @@
 'use strict';
 const common = require('../common');
 const http = require('http');
-const assert = require('assert');
 
 {
   let serverRes;
diff --git a/test/parallel/test-stream-readable-read.js b/test/parallel/test-stream-readable-read.js
new file mode 100644
index 00000000000000..04f68e8d87757f
--- /dev/null
+++ b/test/parallel/test-stream-readable-read.js
@@ -0,0 +1,44 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+
+const { Readable, Transform } = require('stream');
+
+let readCalls = 0;
+class CustomReader extends Readable {
+  constructor(opt) {
+    super(opt);
+    this._max = 1e7;
+    this._index = 0;
+  }
+
+  _read(size) {
+    readCalls++;
+
+    while (this._index < this._max) {
+      this._index++;
+
+      if (this._index >= this._max) {
+        return this.push(null);
+      }
+
+      if (!this.push('a')) {
+        console.log('pause')
+        return;
+      } else {
+        console.log('continue')
+      }
+    }
+  }
+}
+
+new CustomReader()
+  .on('end', common.mustCall())
+  .pipe(new Transform({
+    transform(chunk, encoding, cb) {
+      process.nextTick(cb);
+    }
+  }))
+  .on('finish', common.mustCall(() => {
+    assert.strictEqual(readCalls, 610);
+  }));

From fb43aa25e27f310043a55721855b673fda043263 Mon Sep 17 00:00:00 2001
From: Robert Nagy <ronagy@icloud.com>
Date: Sat, 9 May 2020 10:59:14 +0200
Subject: [PATCH 6/6] fixup

---
 test/parallel/test-stream-readable-read.js | 44 ----------------------
 1 file changed, 44 deletions(-)
 delete mode 100644 test/parallel/test-stream-readable-read.js

diff --git a/test/parallel/test-stream-readable-read.js b/test/parallel/test-stream-readable-read.js
deleted file mode 100644
index 04f68e8d87757f..00000000000000
--- a/test/parallel/test-stream-readable-read.js
+++ /dev/null
@@ -1,44 +0,0 @@
-'use strict';
-const common = require('../common');
-const assert = require('assert');
-
-const { Readable, Transform } = require('stream');
-
-let readCalls = 0;
-class CustomReader extends Readable {
-  constructor(opt) {
-    super(opt);
-    this._max = 1e7;
-    this._index = 0;
-  }
-
-  _read(size) {
-    readCalls++;
-
-    while (this._index < this._max) {
-      this._index++;
-
-      if (this._index >= this._max) {
-        return this.push(null);
-      }
-
-      if (!this.push('a')) {
-        console.log('pause')
-        return;
-      } else {
-        console.log('continue')
-      }
-    }
-  }
-}
-
-new CustomReader()
-  .on('end', common.mustCall())
-  .pipe(new Transform({
-    transform(chunk, encoding, cb) {
-      process.nextTick(cb);
-    }
-  }))
-  .on('finish', common.mustCall(() => {
-    assert.strictEqual(readCalls, 610);
-  }));