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: nodejs/web-server-frameworks#41 PR-URL: #33172 Fixes: #28172 Refs: #28677 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
nodejs · May 10, 2020 · 8a6fab0 · 8a6fab0
1 parent cbf2fa6
commit 8a6fab0
Show file tree

Hide file tree

Showing 8 changed files with 121 additions and 35 deletions.
diff --git 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.
@@ -2417,6 +2416,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: aborted'` and code `'ECONNRESET'`.
 * `'close'`
 * `'close'` on the `res` object
 
@@ -2445,6 +2446,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: aborted'` and code `'ECONNRESET'`.
 * `'close'`
 * `'close'` on the `res` object
 
@@ -2474,6 +2477,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: aborted'` and code `'ECONNRESET'`.
 * `'close'`
 * `'close'` on the `res` object
 

diff --git a/lib/_http_client.js b/lib/_http_client.js
@@ -417,9 +417,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/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 {
@@ -536,9 +540,13 @@ function socketOnClose(socket, state) {
 function abortIncoming(incoming) {
   while (incoming.length) {
     const req = incoming.shift();
+    // TODO(ronag): req.destroy(err)
     req.aborted = true;
     req.destroyed = 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-abort-client.js b/test/parallel/test-http-abort-client.js
@@ -41,6 +41,9 @@ server.listen(0, common.mustCall(() => {
     res.resume();
     res.on('end', common.mustNotCall());
     res.on('aborted', common.mustCall());
+    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
@@ -4,23 +4,58 @@ 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');
+      assert.strictEqual(err.message, 'aborted');
+      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);
+      }));
+      res.on('error', common.expectsError({
+        code: 'ECONNRESET',
+        message: 'aborted'
+      }));
+      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);
+      server.close();
+    }));
+    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();
   }));
-}));
+}
diff --git a/test/parallel/test-http-client-aborted-event.js b/test/parallel/test-http-client-aborted-event.js
@@ -2,19 +2,44 @@
 const common = require('../common');
 const http = require('http');
 
-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.expectsError({
+        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
@@ -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.expectsError({
+        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
@@ -33,6 +33,9 @@ events.captureRejections = true;
 
     req.on('response', common.mustCall((res) => {
       res.on('aborted', common.mustCall());
+      res.on('error', common.expectsError({
+        code: 'ECONNRESET'
+      }));
       res.resume();
       server.close();
     }));