From fdf10adef8443c19899c306804cbe585113ff453 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Mon, 15 Jun 2020 12:59:58 -0700 Subject: [PATCH 1/6] meta: introduce codeowners again The [Contributor's Survey results](https://github.com/nodejs/TSC/pull/882) highlight the fact that it is often not easy for contributors to know who the right people are to talk to about a proposed change or who to ask for reviews of a given subsystem. We briefly toyed around with codeowners before when GitHub introduced it but just as quickly disabled it because the structure of our repository made it exceedingly difficult to get right. Rather than start with a codeowners for the entire project, I propose that we start with a small experiment focused on specific subsystems. Specifically, codeowners for modules, streams, net/tls, http/http2, and quic (once that lands). We can expand out from there as we see how things go with the minimal starter set. Initially this just enables codeowners for the `quic` subsystem. A couple of points: 1. A codeowner should always be a team, never an individual person 2. Each codeowner team should contain at least one TSC member (to provide coverage for signing off on semver-major changes) 3. PRs touching any code with a codeowner must be signed off by at least one person on the codeowner team PR-URL: https://github.com/nodejs/node/pull/33895 Reviewed-By: Matheus Marchini Reviewed-By: Matteo Collina Reviewed-By: Gus Caplan Reviewed-By: Trivikram Kamat --- .github/CODEOWNERS | 76 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100644 .github/CODEOWNERS diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS new file mode 100644 index 00000000000000..6fb4f7dc493b6a --- /dev/null +++ b/.github/CODEOWNERS @@ -0,0 +1,76 @@ +# Node.js Project Codeowners + +# 1. Codeowners must always be teams, never individuals +# 2. Each codeowner team should contain at least one TSC member +# 3. PRs touching any code with a codeowner must be signed off by at least one +# person on the code owner team. + +./.github/CODEOWNERS @nodejs/tsc + +# net + +# ./deps/cares @nodejs/net +# ./doc/api/dns.md @nodejs/net +# ./doc/api/dgram.md @nodejs/net +# ./doc/api/net.md @nodejs/net +# ./lib/dgram.js @nodejs/net +# ./lib/dns.js @nodejs/net +# ./lib/net.js @nodejs/net @nodejs/quic +# ./lib/internal/dgram.js @nodejs/net +# ./lib/internal/dns/* @nodejs/net +# ./lib/internal/net.js @nodejs/net +# ./lib/internal/socket_list.js @nodejs/net +# ./lib/internal/js_stream_socket.js @nodejs/net +# ./src/cares_wrap.h @nodejs/net +# ./src/connect_wrap.* @nodejs/net +# ./src/connection_wrap.* @nodejs/net +# ./src/node_sockaddr* @nodejs/net +# ./src/tcp_wrap.* @nodejs/net +# ./src/udp_wrap.* @nodejs/net + +# tls/crypto + +# ./lib/internal/crypto/* @nodejs/crypto +# ./lib/internal/tls.js @nodejs/crypto @nodejs/net +# ./lib/crypto.js @nodejs/crypto +# ./lib/tls.js @nodejs/crypto @nodejs/net +# ./src/node_crypto* @nodejs/crypto +# ./src/node_crypto_common* @nodejs/crypto @nodejs/quic + +# http + +# ./deps/llhttp/* @nodejs/http @nodejs/net +# ./doc/api/http.md @nodejs/http @nodejs/net +# ./doc/api/http2.md @nodejs/http @nodejs/net +# ./lib/_http_* @nodejs/http @nodejs/net +# ./lib/http.js @nodejs/http @nodejs/net +# ./lib/https.js @nodejs/crypto @nodejs/net @nodejs/http +# ./src/node_http_common* @nodejs/http @nodejs/http2 @nodejs/quic @nodejs/net +# ./src/node_http_parser.cc @nodejs/http @nodejs/net + +# http2 + +# ./deps/nghttp2/* @nodejs/http2 @nodejs/net +# ./doc/api/http2.md @nodejs/http2 @nodejs/net +# ./lib/http2.js @nodejs/http2 @nodejs/net +# ./lib/internal/http2/* @nodejs/http2 @nodejs/net +# ./src/node_http2* @nodejs/http2 @nodejs/net +# ./src/node_mem* @nodejs/http2 + +# quic + +./deps/ngtcp2/* @nodejs/quic +./deps/nghttp3/* @nodejs/quic +./doc/api/quic.md @nodejs/quic +./lib/internal/quic/* @nodejs/quic +./src/node_bob* @nodejs/quic +./src/quic/* @nodejs/quic + +# modules + +# ./doc/api/modules.md @nodejs/modules +# ./doc/api/esm.md @nodejs/modules +# ./lib/module.js @nodejs/modules +# ./lib/internal/modules/* @nodejs/modules +# ./lib/internal/bootstrap/loaders.js @nodejs/modules +# ./src/module_wrap* @nodejs/modules @nodejs/vm From 30cc54275d570a804ced31843d1ff237dd701f85 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sat, 30 May 2020 10:40:30 +0200 Subject: [PATCH 2/6] 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 3093047bd731f2..34ab5400207f45 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 dade9a11014c01..0fa2a386eb17be 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 f11c74ff7c4303..ee763daf889a28 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 aab4170a2f0e06..f7ed50d0c0f3cb 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 91c68aa9af3db0..47d5e948ab7970 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 00000000000000..e0199a1cd5ef23 --- /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 589b673282c517..ba2877131227aa 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 37fbe062f12b69..02f86f611c1b8e 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(); + })); })); } From 51a2df4439ea23b5121fcb8e674eb7a3b144cdfa Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sat, 20 Jun 2020 00:12:44 +0200 Subject: [PATCH 3/6] fs: document why isPerformingIO is required PR-URL: https://github.com/nodejs/node/pull/33982 Reviewed-By: Anna Henningsen Reviewed-By: Trivikram Kamat Reviewed-By: Ben Noordhuis Reviewed-By: Luigi Pinca --- lib/internal/fs/streams.js | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/lib/internal/fs/streams.js b/lib/internal/fs/streams.js index 9e6050139dc79a..0209f3e844c759 100644 --- a/lib/internal/fs/streams.js +++ b/lib/internal/fs/streams.js @@ -255,6 +255,12 @@ ReadStream.prototype._read = function(n) { }; ReadStream.prototype._destroy = function(err, cb) { + // Usually for async IO it is safe to close a file descriptor + // even when there are pending operations. However, due to platform + // differences file IO is implemented using synchronous operations + // running in a thread pool. Therefore, file descriptors are not safe + // to close while used in a pending read or write operation. Wait for + // any pending IO (kIsPerformingIO) to complete (kIoDone). if (this[kIsPerformingIO]) { this.once(kIoDone, (er) => close(this, err || er, cb)); } else { @@ -416,12 +422,19 @@ WriteStream.prototype._writev = function(data, cb) { }; WriteStream.prototype._destroy = function(err, cb) { + // Usually for async IO it is safe to close a file descriptor + // even when there are pending operations. However, due to platform + // differences file IO is implemented using synchronous operations + // running in a thread pool. Therefore, file descriptors are not safe + // to close while used in a pending read or write operation. Wait for + // any pending IO (kIsPerformingIO) to complete (kIoDone). if (this[kIsPerformingIO]) { this.once(kIoDone, (er) => close(this, err || er, cb)); } else { close(this, err, cb); } }; + WriteStream.prototype.close = function(cb) { if (cb) { if (this.closed) { From 64d22c320c67255b42c8185a333a92a499c95820 Mon Sep 17 00:00:00 2001 From: Denys Otrishko Date: Thu, 18 Jun 2020 22:58:42 +0300 Subject: [PATCH 4/6] timers: fix multipleResolves in promisified timeouts/immediates After successful timer finish the abort event callback would still reject (already resolved promise) upon calling abortController.abort(). Signed-off-by: Denys Otrishko PR-URL: https://github.com/nodejs/node/pull/33949 Reviewed-By: James M Snell Reviewed-By: Benjamin Gruenbaum Reviewed-By: Anna Henningsen --- lib/timers.js | 12 ++++++++---- test/parallel/test-timers-promisified.js | 19 +++++++++++++++++++ 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/lib/timers.js b/lib/timers.js index fce904019205e9..53e934f3c9ac7c 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -190,8 +190,10 @@ setTimeout[customPromisify] = function(after, value, options = {}) { insert(timeout, timeout._idleTimeout); if (signal) { signal.addEventListener('abort', () => { - clearTimeout(timeout); - reject(lazyDOMException('AbortError')); + if (!timeout._destroyed) { + clearTimeout(timeout); + reject(lazyDOMException('AbortError')); + } }, { once: true }); } }); @@ -340,8 +342,10 @@ setImmediate[customPromisify] = function(value, options = {}) { const immediate = new Immediate(resolve, [value]); if (signal) { signal.addEventListener('abort', () => { - clearImmediate(immediate); - reject(lazyDOMException('AbortError')); + if (!immediate._destroyed) { + clearImmediate(immediate); + reject(lazyDOMException('AbortError')); + } }, { once: true }); } }); diff --git a/test/parallel/test-timers-promisified.js b/test/parallel/test-timers-promisified.js index d470d2f97c79eb..f46dc3f24d20eb 100644 --- a/test/parallel/test-timers-promisified.js +++ b/test/parallel/test-timers-promisified.js @@ -10,6 +10,8 @@ const { promisify } = require('util'); const setTimeout = promisify(timers.setTimeout); const setImmediate = promisify(timers.setImmediate); +process.on('multipleResolves', common.mustNotCall()); + { const promise = setTimeout(1); promise.then(common.mustCall((value) => { @@ -66,6 +68,23 @@ const setImmediate = promisify(timers.setImmediate); assert.rejects(setImmediate(10, { signal }), /AbortError/); } +{ + // Check that aborting after resolve will not reject. + const ac = new AbortController(); + const signal = ac.signal; + setTimeout(10, undefined, { signal }).then(() => { + ac.abort(); + }); +} +{ + // Check that aborting after resolve will not reject. + const ac = new AbortController(); + const signal = ac.signal; + setImmediate(10, { signal }).then(() => { + ac.abort(); + }); +} + { Promise.all( [1, '', false, Infinity].map((i) => assert.rejects(setImmediate(10, i)), { From 7a5d3a2fc1915166b1da2c161f020358a8c36bfd Mon Sep 17 00:00:00 2001 From: rickyes Date: Thu, 28 May 2020 23:15:55 +0800 Subject: [PATCH 5/6] http: add maxTotalSockets to agent class Add maxTotalSockets to determine how many sockets an agent can open. Unlike maxSockets, The maxTotalSockets does not count by per origin. PR-URL: https://github.com/nodejs/node/pull/33617 Fixes: https://github.com/nodejs/node/issues/31942 Reviewed-By: Robert Nagy Reviewed-By: Matteo Collina Reviewed-By: James M Snell --- doc/api/http.md | 10 ++ lib/_http_agent.js | 52 +++++++- .../test-http-agent-maxtotalsockets.js | 113 ++++++++++++++++++ 3 files changed, 171 insertions(+), 4 deletions(-) create mode 100644 test/parallel/test-http-agent-maxtotalsockets.js diff --git a/doc/api/http.md b/doc/api/http.md index b48874c9fcec21..c3e2d016608aed 100644 --- a/doc/api/http.md +++ b/doc/api/http.md @@ -300,6 +300,16 @@ added: v0.3.6 By default set to `Infinity`. Determines how many concurrent sockets the agent can have open per origin. Origin is the returned value of [`agent.getName()`][]. +### `agent.maxTotalSockets` + + +* {number} + +By default set to `Infinity`. Determines how many concurrent sockets the agent +can have open. Unlike `maxSockets`, this parameter applies across all origins. + ### `agent.requests`