From 30cc54275d570a804ced31843d1ff237dd701f85 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sat, 30 May 2020 10:40:30 +0200 Subject: [PATCH] http: don't emit error after close Refs: https://github.com/nodejs/node/issues/33591 PR-URL: https://github.com/nodejs/node/pull/33654 Reviewed-By: James M Snell Reviewed-By: Matteo Collina --- lib/_http_client.js | 7 +++++- lib/_http_outgoing.js | 13 +++++++--- lib/_http_server.js | 3 +++ lib/internal/http.js | 1 + test/parallel/test-http-outgoing-destroy.js | 7 +----- test/parallel/test-http-outgoing-destroyed.js | 24 +++++++++++++++++++ .../test-http-server-write-after-end.js | 16 ++++++------- .../test-http-server-write-end-after-end.js | 18 ++++++++------ 8 files changed, 64 insertions(+), 25 deletions(-) create mode 100644 test/parallel/test-http-outgoing-destroyed.js diff --git a/lib/_http_client.js b/lib/_http_client.js index 3093047bd73..34ab5400207 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -49,7 +49,7 @@ const Agent = require('_http_agent'); const { Buffer } = require('buffer'); const { defaultTriggerAsyncIdScope } = require('internal/async_hooks'); const { URL, urlToOptions, searchParamsSymbol } = require('internal/url'); -const { kOutHeaders, kNeedDrain } = require('internal/http'); +const { kOutHeaders, kNeedDrain, kClosed } = require('internal/http'); const { connResetException, codes } = require('internal/errors'); const { ERR_HTTP_HEADERS_SENT, @@ -385,6 +385,7 @@ function _destroy(req, socket, err) { if (err) { req.emit('error', err); } + req[kClosed] = true; req.emit('close'); } } @@ -427,6 +428,7 @@ function socketCloseListener() { res.emit('error', connResetException('aborted')); } } + req[kClosed] = true; req.emit('close'); if (!res.aborted && res.readable) { res.on('end', function() { @@ -446,6 +448,7 @@ function socketCloseListener() { req.socket._hadError = true; req.emit('error', connResetException('socket hang up')); } + req[kClosed] = true; req.emit('close'); } @@ -550,6 +553,7 @@ function socketOnData(d) { req.emit(eventName, res, socket, bodyHead); req.destroyed = true; + req[kClosed] = true; req.emit('close'); } else { // Requested Upgrade or used CONNECT method, but have no handler. @@ -721,6 +725,7 @@ function requestOnPrefinish() { } function emitFreeNT(req) { + req[kClosed] = true; req.emit('close'); if (req.res) { req.res.emit('close'); diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index dade9a11014..0fa2a386eb1 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -36,7 +36,7 @@ const assert = require('internal/assert'); const EE = require('events'); const Stream = require('stream'); const internalUtil = require('internal/util'); -const { kOutHeaders, utcDate, kNeedDrain } = require('internal/http'); +const { kOutHeaders, utcDate, kNeedDrain, kClosed } = require('internal/http'); const { Buffer } = require('buffer'); const common = require('_http_common'); const checkIsHttpToken = common._checkIsHttpToken; @@ -117,6 +117,7 @@ function OutgoingMessage() { this.finished = false; this._headerSent = false; this[kCorked] = 0; + this[kClosed] = false; this.socket = null; this._header = null; @@ -663,7 +664,9 @@ function onError(msg, err, callback) { function emitErrorNt(msg, err, callback) { callback(err); - if (typeof msg.emit === 'function') msg.emit('error', err); + if (typeof msg.emit === 'function' && !msg[kClosed]) { + msg.emit('error', err); + } } function write_(msg, chunk, encoding, callback, fromEnd) { @@ -690,7 +693,11 @@ function write_(msg, chunk, encoding, callback, fromEnd) { } if (err) { - onError(msg, err, callback); + if (!msg.destroyed) { + onError(msg, err, callback); + } else { + process.nextTick(callback, err); + } return false; } diff --git a/lib/_http_server.js b/lib/_http_server.js index f11c74ff7c4..ee763daf889 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -49,6 +49,7 @@ const { OutgoingMessage } = require('_http_outgoing'); const { kOutHeaders, kNeedDrain, + kClosed, emitStatistics } = require('internal/http'); const { @@ -212,6 +213,7 @@ function onServerResponseClose() { // Fortunately, that requires only a single if check. :-) if (this._httpMessage) { this._httpMessage.destroyed = true; + this._httpMessage[kClosed] = true; this._httpMessage.emit('close'); } } @@ -729,6 +731,7 @@ function resOnFinish(req, res, socket, state, server) { function emitCloseNT(self) { self.destroyed = true; + self[kClosed] = true; self.emit('close'); } diff --git a/lib/internal/http.js b/lib/internal/http.js index aab4170a2f0..f7ed50d0c0f 100644 --- a/lib/internal/http.js +++ b/lib/internal/http.js @@ -51,6 +51,7 @@ function emitStatistics(statistics) { module.exports = { kOutHeaders: Symbol('kOutHeaders'), kNeedDrain: Symbol('kNeedDrain'), + kClosed: Symbol('kClosed'), nowDate, utcDate, emitStatistics diff --git a/test/parallel/test-http-outgoing-destroy.js b/test/parallel/test-http-outgoing-destroy.js index 91c68aa9af3..47d5e948ab7 100644 --- a/test/parallel/test-http-outgoing-destroy.js +++ b/test/parallel/test-http-outgoing-destroy.js @@ -10,13 +10,8 @@ const OutgoingMessage = http.OutgoingMessage; assert.strictEqual(msg.destroyed, false); msg.destroy(); assert.strictEqual(msg.destroyed, true); - let callbackCalled = false; msg.write('asd', common.mustCall((err) => { assert.strictEqual(err.code, 'ERR_STREAM_DESTROYED'); - callbackCalled = true; - })); - msg.on('error', common.mustCall((err) => { - assert.strictEqual(err.code, 'ERR_STREAM_DESTROYED'); - assert.strictEqual(callbackCalled, true); })); + msg.on('error', common.mustNotCall()); } diff --git a/test/parallel/test-http-outgoing-destroyed.js b/test/parallel/test-http-outgoing-destroyed.js new file mode 100644 index 00000000000..e0199a1cd5e --- /dev/null +++ b/test/parallel/test-http-outgoing-destroyed.js @@ -0,0 +1,24 @@ +'use strict'; +const common = require('../common'); +const http = require('http'); + +const server = http.createServer(common.mustCall((req, res) => { + req.pipe(res); + res.on('error', common.mustNotCall()); + res.on('close', common.mustCall(() => { + res.end('asd'); + process.nextTick(() => { + server.close(); + }); + })); +})).listen(0, () => { + http + .request({ + port: server.address().port, + method: 'PUT' + }) + .on('response', (res) => { + res.destroy(); + }) + .write('asd'); +}); diff --git a/test/parallel/test-http-server-write-after-end.js b/test/parallel/test-http-server-write-after-end.js index 589b673282c..ba287713122 100644 --- a/test/parallel/test-http-server-write-after-end.js +++ b/test/parallel/test-http-server-write-after-end.js @@ -8,19 +8,19 @@ const http = require('http'); const server = http.createServer(handle); function handle(req, res) { - res.on('error', common.mustCall((err) => { - common.expectsError({ - code: 'ERR_STREAM_WRITE_AFTER_END', - name: 'Error' - })(err); - server.close(); - })); + res.on('error', common.mustNotCall()); res.write('hello'); res.end(); setImmediate(common.mustCall(() => { - res.write('world'); + res.write('world', common.mustCall((err) => { + common.expectsError({ + code: 'ERR_STREAM_WRITE_AFTER_END', + name: 'Error' + })(err); + server.close(); + })); })); } diff --git a/test/parallel/test-http-server-write-end-after-end.js b/test/parallel/test-http-server-write-end-after-end.js index 37fbe062f12..02f86f611c1 100644 --- a/test/parallel/test-http-server-write-end-after-end.js +++ b/test/parallel/test-http-server-write-end-after-end.js @@ -6,19 +6,23 @@ const http = require('http'); const server = http.createServer(handle); function handle(req, res) { - res.on('error', common.mustCall((err) => { - common.expectsError({ - code: 'ERR_STREAM_WRITE_AFTER_END', - name: 'Error' - })(err); - server.close(); - })); + res.on('error', common.mustNotCall()); res.write('hello'); res.end(); setImmediate(common.mustCall(() => { res.end('world'); + process.nextTick(() => { + server.close(); + }); + res.write('world', common.mustCall((err) => { + common.expectsError({ + code: 'ERR_STREAM_WRITE_AFTER_END', + name: 'Error' + })(err); + server.close(); + })); })); }