From e8a134190e85d0a1cf963eb3e5b76161e5ca85de Mon Sep 17 00:00:00 2001 From: Dan Bornstein Date: Tue, 21 Mar 2023 09:33:45 -0700 Subject: [PATCH 01/17] Expose the underlying socket timeout. --- .../private/RateLimitedStream.js | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/builtin-services/private/RateLimitedStream.js b/src/builtin-services/private/RateLimitedStream.js index a8acbf91c..110a7b398 100644 --- a/src/builtin-services/private/RateLimitedStream.js +++ b/src/builtin-services/private/RateLimitedStream.js @@ -338,6 +338,23 @@ export class RateLimitedStream { get remotePort() { return this.#outerThis.#innerStream.remotePort; } + + /** + * @returns {number} The idle-timeout time, in msec. `0` indicates that + * timeout is disabled. + */ + get timeout() { + return this.#outerThis.#innerStream.timeout; + } + + /** + * @param {number} timeoutMsec The new idle-timeout time, in msec. `0` + * indicates that timeout is disabled. + */ + set timeout(timeoutMsec) { + MustBe.number(timeoutMsec, { finite: true, minInclusive: 0 }); + this.#outerThis.#innerStream.timeout = timeoutMsec; + } }; /** From 5f95c8ed17e356f702d697a13e4d320640f29bcb Mon Sep 17 00:00:00 2001 From: Dan Bornstein Date: Tue, 21 Mar 2023 09:35:19 -0700 Subject: [PATCH 02/17] Tweaks in prep of further changes. --- .../private/RateLimitedStream.js | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/builtin-services/private/RateLimitedStream.js b/src/builtin-services/private/RateLimitedStream.js index 110a7b398..104a59a5d 100644 --- a/src/builtin-services/private/RateLimitedStream.js +++ b/src/builtin-services/private/RateLimitedStream.js @@ -101,22 +101,24 @@ export class RateLimitedStream { * @returns {Duplex|Writable} The wrapper. */ #createWrapper() { - const inner = this.#innerStream; - const hasReader = inner instanceof Readable; + const inner = this.#innerStream; + const isReader = inner instanceof Readable; + const isSocket = inner instanceof Socket; inner.on('close', () => this.#writableOnClose()); inner.on('error', (error) => this.#onError(error)); - if (hasReader) { - // Note: Adding the `readable` listener causes the stream to become - // "paused" (that is, it won't spontaneously emit `data` events). + if (isReader) { + // Note: Adding the `readable` listener (as is done here) causes the + // stream to become "paused" (that is, it won't spontaneously emit `data` + // events). inner.on('end', () => this.#readableOnEnd()); inner.on('readable', () => this.#readableOnReadable()); } - if (inner instanceof Socket) { + if (isSocket) { return new RateLimitedStream.#SocketWrapper(this); - } else if (inner instanceof Readable) { + } else if (isReadable) { return new RateLimitedStream.#DuplexWrapper(this); } else { return new RateLimitedStream.#WritableWrapper(this); From 84bf94943a27257ea2211c4dc9d78e229f5e7cde Mon Sep 17 00:00:00 2001 From: Dan Bornstein Date: Tue, 21 Mar 2023 09:38:15 -0700 Subject: [PATCH 03/17] Plumb `timeout` events through to the wrapper. --- src/builtin-services/private/RateLimitedStream.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/builtin-services/private/RateLimitedStream.js b/src/builtin-services/private/RateLimitedStream.js index 104a59a5d..dda2ca508 100644 --- a/src/builtin-services/private/RateLimitedStream.js +++ b/src/builtin-services/private/RateLimitedStream.js @@ -116,6 +116,10 @@ export class RateLimitedStream { inner.on('readable', () => this.#readableOnReadable()); } + if (isSocket) { + inner.on('timeout', () => this.#outerStream.emit('timeout')); + } + if (isSocket) { return new RateLimitedStream.#SocketWrapper(this); } else if (isReadable) { From 5825694171d6be18590e50fe139c519a9a4f7a94 Mon Sep 17 00:00:00 2001 From: Dan Bornstein Date: Tue, 21 Mar 2023 09:38:55 -0700 Subject: [PATCH 04/17] Fix name. --- src/builtin-services/private/RateLimitedStream.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/builtin-services/private/RateLimitedStream.js b/src/builtin-services/private/RateLimitedStream.js index dda2ca508..21129a3ea 100644 --- a/src/builtin-services/private/RateLimitedStream.js +++ b/src/builtin-services/private/RateLimitedStream.js @@ -101,14 +101,14 @@ export class RateLimitedStream { * @returns {Duplex|Writable} The wrapper. */ #createWrapper() { - const inner = this.#innerStream; - const isReader = inner instanceof Readable; - const isSocket = inner instanceof Socket; + const inner = this.#innerStream; + const isReadable = inner instanceof Readable; + const isSocket = inner instanceof Socket; inner.on('close', () => this.#writableOnClose()); inner.on('error', (error) => this.#onError(error)); - if (isReader) { + if (isReadable) { // Note: Adding the `readable` listener (as is done here) causes the // stream to become "paused" (that is, it won't spontaneously emit `data` // events). From fce74324c1cf555cda92626fb630f58f415f45f3 Mon Sep 17 00:00:00 2001 From: Dan Bornstein Date: Tue, 21 Mar 2023 09:39:38 -0700 Subject: [PATCH 05/17] Better wording. --- src/builtin-services/private/RateLimitedStream.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/builtin-services/private/RateLimitedStream.js b/src/builtin-services/private/RateLimitedStream.js index 21129a3ea..c206d145b 100644 --- a/src/builtin-services/private/RateLimitedStream.js +++ b/src/builtin-services/private/RateLimitedStream.js @@ -109,9 +109,9 @@ export class RateLimitedStream { inner.on('error', (error) => this.#onError(error)); if (isReadable) { - // Note: Adding the `readable` listener (as is done here) causes the - // stream to become "paused" (that is, it won't spontaneously emit `data` - // events). + // Note: Adding a listener for the `readable` event (as is done here) + // causes the stream to become "paused" (that is, it won't spontaneously + // emit `data` events). inner.on('end', () => this.#readableOnEnd()); inner.on('readable', () => this.#readableOnReadable()); } From 92b44040b1ed3538e236adfa6227c72efe86b812 Mon Sep 17 00:00:00 2001 From: Dan Bornstein Date: Tue, 21 Mar 2023 10:12:29 -0700 Subject: [PATCH 06/17] Implement socket timeouts for all TCP connections, in one place. ...and more robustly too, I hope. --- src/network-protocol/private/TcpWrangler.js | 67 +++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/src/network-protocol/private/TcpWrangler.js b/src/network-protocol/private/TcpWrangler.js index 07465c5ba..fa52fb134 100644 --- a/src/network-protocol/private/TcpWrangler.js +++ b/src/network-protocol/private/TcpWrangler.js @@ -2,6 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 import * as net from 'node:net'; +import * as timers from 'node:timers/promises'; import { Condition, Threadlet } from '@this/async'; import { FormatUtils, IntfLogger } from '@this/loggy'; @@ -138,6 +139,11 @@ export class TcpWrangler extends ProtocolWrangler { this.#sockets.add(socket); this.#anySockets.value = true; + socket.timeout = TcpWrangler.#SOCKET_TIMEOUT_MSEC; + socket.on('timeout', async () => { + this.#handleTimeout(socket, connLogger); + }); + socket.on('error', (error) => { // A `close` event gets emitted right after this event -- which performs // connection cleanup -- so there's no need to do anything other than log @@ -171,6 +177,54 @@ export class TcpWrangler extends ProtocolWrangler { this.#logger?.droppedConnection(data); } + /** + * Handles a timed out socket. + * + * @param {Socket} socket The socket that timed out. + * @param {?IntfLogger} logger Logger to use, if any. + */ + async #handleTimeout(socket, logger) { + logger = logger?.socketTimeout; + + if (socket.destroyed) { + logger?.alreadyDestroyed(); + return; + } + + const closedCond = new Condition(); + + logger?.closing(); + socket.destroySoon(); + socket.once('close', () => { + closedCond.value = true; + logger?.closed(); + }); + + await Promise.race([ + closedCond.whenTrue(), + timers.setTimeout(TcpWrangler.SOCKET_TIMEOUT_CLOSE_GRACE_PERIOD_MSEC) + ]); + + if (socket.destroyed) { + logger?.destroyed(); + return; + } + + logger?.destroyingForcefully(); + socket.destroy(); + + await Promise.race([ + closedCond.whenTrue(), + timers.setTimeout(TcpWrangler.SOCKET_TIMEOUT_CLOSE_GRACE_PERIOD_MSEC) + ]); + + if (socket.destroyed) { + logger?.destroyed(); + } else { + logger?.givingUp(); + } + } + /** * Runs the low-level stack. This is called as the main function of the * {@link #runner}. @@ -281,6 +335,19 @@ export class TcpWrangler extends ProtocolWrangler { port: null }); + /** + * @type {number} How long in msec to wait before considering a connected + * socket (a/o/t a server socket doing a `listen()`) to be "timed out." When + * timed out, a socket is closed proactively. + */ + static #SOCKET_TIMEOUT_MSEC = 3 * 60 * 1000; // Three minutes. + + /** + * @type {number} Grace period in msec after trying to close a socket due to + * timeout, before doing it more forcefully. + */ + static #SOCKET_TIMEOUT_CLOSE_GRACE_PERIOD_MSEC = 250; // Quarter of a second. + /** * Trims down and "fixes" `options` using the given prototype. This is used * to convert from our incoming `interface` form to what's expected by Node's From 8ae72096c1fab0e90308f4b052503f57ab6224f4 Mon Sep 17 00:00:00 2001 From: Dan Bornstein Date: Tue, 21 Mar 2023 10:16:53 -0700 Subject: [PATCH 07/17] Move the salient note. --- src/network-protocol/private/TcpWrangler.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/network-protocol/private/TcpWrangler.js b/src/network-protocol/private/TcpWrangler.js index fa52fb134..90576c37b 100644 --- a/src/network-protocol/private/TcpWrangler.js +++ b/src/network-protocol/private/TcpWrangler.js @@ -139,6 +139,13 @@ export class TcpWrangler extends ProtocolWrangler { this.#sockets.add(socket); this.#anySockets.value = true; + // Note: Doing a socket timeout is a good idea in general. But beyond that, + // as of this writing, there's a bug in Node which causes it to consistently + // leak memory when sockets aren't proactively timed out, see issue #42710 + // . We have observed memory + // leakage consistent with the issue in this project, and the working + // hypothesis is that setting this timeout will suffice as a fix / + // workaround (depending on one's perspective). socket.timeout = TcpWrangler.#SOCKET_TIMEOUT_MSEC; socket.on('timeout', async () => { this.#handleTimeout(socket, connLogger); From 5caf179a221608e4b69a40a3eb799b86f39fa058 Mon Sep 17 00:00:00 2001 From: Dan Bornstein Date: Tue, 21 Mar 2023 10:18:14 -0700 Subject: [PATCH 08/17] Drop the earlier attempt at timeouts. --- src/network-protocol/private/Http2Wrangler.js | 23 ------------------- src/network-protocol/private/HttpWrangler.js | 23 ------------------- src/network-protocol/private/HttpsWrangler.js | 23 ------------------- 3 files changed, 69 deletions(-) diff --git a/src/network-protocol/private/Http2Wrangler.js b/src/network-protocol/private/Http2Wrangler.js index 1b0d6b6fb..0046785f4 100644 --- a/src/network-protocol/private/Http2Wrangler.js +++ b/src/network-protocol/private/Http2Wrangler.js @@ -57,23 +57,6 @@ export class Http2Wrangler extends TcpWrangler { this.#protocolServer = http2.createSecureServer(serverOptions); this.#protocolServer.on('session', (session) => this.#addSession(session)); - - // Explicitly set the default socket timeout, as doing this _might_ - // mitigate a memory leak as noted in - // . As of this writing, there - // _is_ a memory leak of some sort in this project, and the working - // hypothesis is that setting this timeout will suffice as a fix / - // workaround (depending on one's perspective). - // **Note:** `server.setTimeout(msec)` and `server.timeout = msec` do the - // same thing (though the former can be used with an extra argument to - // set up a callback at the same time). - this.#protocolServer.timeout = Http2Wrangler.#SOCKET_TIMEOUT_MSEC; - - // TODO: Either remove this entirely, if it turns out that the server - // timeout is useless (for us), or add something useful here. - this.#protocolServer.on('timeout', () => { - this.#logger?.serverTimeout(); - }); } /** @override */ @@ -254,10 +237,4 @@ export class Http2Wrangler extends TcpWrangler { * before considering it "timed out" and telling it to close. */ static #SESSION_TIMEOUT_MSEC = 1 * 60 * 1000; // One minute. - - /** - * @type {number} How long in msec to wait before considering a socket - * "timed out." - */ - static #SOCKET_TIMEOUT_MSEC = 3 * 60 * 1000; // Three minutes. } diff --git a/src/network-protocol/private/HttpWrangler.js b/src/network-protocol/private/HttpWrangler.js index 6d8aeeb12..630693a5d 100644 --- a/src/network-protocol/private/HttpWrangler.js +++ b/src/network-protocol/private/HttpWrangler.js @@ -34,18 +34,6 @@ export class HttpWrangler extends TcpWrangler { this.#logger = options.logger?.http ?? null; this.#application = express(); this.#protocolServer = http.createServer(); - - // Explicitly set the default socket timeout, as doing this _might_ help - // prevent memory leaks. See the longer comment in the `Http2Wrangler` - // constructor for details. The bug noted there is HTTP2-specific, but the - // possibility of socket leakage seems like it could easily happen here too. - this.#protocolServer.timeout = HttpWrangler.#SOCKET_TIMEOUT_MSEC; - - // TODO: Either remove this entirely, if it turns out that the server - // timeout is useless (for us), or add something useful here. - this.#protocolServer.on('timeout', () => { - this.#logger?.serverTimeout(); - }); } /** @override */ @@ -71,15 +59,4 @@ export class HttpWrangler extends TcpWrangler { _impl_server() { return this.#protocolServer; } - - - // - // Static members - // - - /** - * @type {number} How long in msec to wait before considering a socket - * "timed out." - */ - static #SOCKET_TIMEOUT_MSEC = 3 * 60 * 1000; // Three minutes. } diff --git a/src/network-protocol/private/HttpsWrangler.js b/src/network-protocol/private/HttpsWrangler.js index 50de18e23..dd374e282 100644 --- a/src/network-protocol/private/HttpsWrangler.js +++ b/src/network-protocol/private/HttpsWrangler.js @@ -34,18 +34,6 @@ export class HttpsWrangler extends TcpWrangler { this.#logger = options.logger?.https ?? null; this.#application = express(); this.#protocolServer = https.createServer(options.hosts); - - // Explicitly set the default socket timeout, as doing this _might_ help - // prevent memory leaks. See the longer comment in the `Http2Wrangler` - // constructor for details. The bug noted there is HTTP2-specific, but the - // possibility of socket leakage seems like it could easily happen here too. - this.#protocolServer.timeout = HttpsWrangler.#SOCKET_TIMEOUT_MSEC; - - // TODO: Either remove this entirely, if it turns out that the server - // timeout is useless (for us), or add something useful here. - this.#protocolServer.on('timeout', () => { - this.#logger?.serverTimeout(); - }); } /** @override */ @@ -71,15 +59,4 @@ export class HttpsWrangler extends TcpWrangler { _impl_server() { return this.#protocolServer; } - - - // - // Static members - // - - /** - * @type {number} How long in msec to wait before considering a socket - * "timed out." - */ - static #SOCKET_TIMEOUT_MSEC = 3 * 60 * 1000; // Three minutes. } From 7f7717b993a75dcf0291af150775c2b8adfde990 Mon Sep 17 00:00:00 2001 From: Dan Bornstein Date: Tue, 21 Mar 2023 10:21:08 -0700 Subject: [PATCH 09/17] Fix comments. --- src/network-protocol/private/TcpWrangler.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/network-protocol/private/TcpWrangler.js b/src/network-protocol/private/TcpWrangler.js index 90576c37b..d8f028a6e 100644 --- a/src/network-protocol/private/TcpWrangler.js +++ b/src/network-protocol/private/TcpWrangler.js @@ -187,7 +187,7 @@ export class TcpWrangler extends ProtocolWrangler { /** * Handles a timed out socket. * - * @param {Socket} socket The socket that timed out. + * @param {net.Socket} socket The socket that timed out. * @param {?IntfLogger} logger Logger to use, if any. */ async #handleTimeout(socket, logger) { @@ -358,7 +358,7 @@ export class TcpWrangler extends ProtocolWrangler { /** * Trims down and "fixes" `options` using the given prototype. This is used * to convert from our incoming `interface` form to what's expected by Node's - * `net.server`. + * `Server` creation methods. * * @param {object} options Original options. * @param {object} proto The "prototype" for what bindings to keep. From 227314de6b832915e36d8d16c098e970706e6d5b Mon Sep 17 00:00:00 2001 From: Dan Bornstein Date: Tue, 21 Mar 2023 10:22:43 -0700 Subject: [PATCH 10/17] Import better, maybe. --- src/network-protocol/private/TcpWrangler.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/network-protocol/private/TcpWrangler.js b/src/network-protocol/private/TcpWrangler.js index d8f028a6e..091119fe7 100644 --- a/src/network-protocol/private/TcpWrangler.js +++ b/src/network-protocol/private/TcpWrangler.js @@ -1,7 +1,7 @@ // Copyright 2022-2023 the Lactoserv Authors (Dan Bornstein et alia). // SPDX-License-Identifier: Apache-2.0 -import * as net from 'node:net'; +import { Server, Socket, createServer as netCreateServer } from 'node:net'; import * as timers from 'node:timers/promises'; import { Condition, Threadlet } from '@this/async'; @@ -22,7 +22,7 @@ export class TcpWrangler extends ProtocolWrangler { /** @type {?IntfRateLimiter} Rate limiter service to use, if any. */ #rateLimiter; - /** @type {net.Server} Server socket, per se. */ + /** @type {Server} Server socket, per se. */ #serverSocket; /** @type {object} Server socket `listen()` options. */ @@ -55,7 +55,7 @@ export class TcpWrangler extends ProtocolWrangler { this.#logger = options.logger ?? null; this.#rateLimiter = options.rateLimiter ?? null; - this.#serverSocket = net.createServer(serverOptions); + this.#serverSocket = netCreateServer(serverOptions); this.#listenOptions = listenOptions; this.#loggableInfo = { interface: FormatUtils.networkInterfaceString(options.interface), @@ -98,7 +98,7 @@ export class TcpWrangler extends ProtocolWrangler { * manually. This is a relatively small price to pay for getting to be able to * have visibility on the actual network traffic. * - * @param {net.Socket} socket Socket for the newly-opened connection. + * @param {Socket} socket Socket for the newly-opened connection. * @param {...*} rest Any other arguments that happened to be be part of the * `connection` event. */ @@ -187,7 +187,7 @@ export class TcpWrangler extends ProtocolWrangler { /** * Handles a timed out socket. * - * @param {net.Socket} socket The socket that timed out. + * @param {Socket} socket The socket that timed out. * @param {?IntfLogger} logger Logger to use, if any. */ async #handleTimeout(socket, logger) { From c8d194a2377034c668bc8f0a48f188d146e6c87c Mon Sep 17 00:00:00 2001 From: Dan Bornstein Date: Tue, 21 Mar 2023 10:24:41 -0700 Subject: [PATCH 11/17] Changelog and version. --- CHANGELOG.md | 6 ++++++ src/main-lactoserv/package.json | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a3d0e2e4b..471edf323 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,12 @@ Changelog ========= +### v0.5.6 -- 2023-03-21 + +Notable changes: + +* A more holistic attempt at doing socket timeouts. + ### v0.5.5 -- 2023-03-20 Notable changes: diff --git a/src/main-lactoserv/package.json b/src/main-lactoserv/package.json index 308ccfd52..e1977044d 100644 --- a/src/main-lactoserv/package.json +++ b/src/main-lactoserv/package.json @@ -1,6 +1,6 @@ { "name": "@this/main-lactoserv", - "version": "0.5.5", + "version": "0.5.6", "type": "module", "private": true, "license": "Apache-2.0", From 829174fc1de6f66e63b57c8a482e6768bcff02c9 Mon Sep 17 00:00:00 2001 From: Dan Bornstein Date: Tue, 21 Mar 2023 11:10:34 -0700 Subject: [PATCH 12/17] Fix timeout plumbing, and add `destroySoon()`. --- .../private/RateLimitedStream.js | 36 ++++++++++++++++--- 1 file changed, 32 insertions(+), 4 deletions(-) diff --git a/src/builtin-services/private/RateLimitedStream.js b/src/builtin-services/private/RateLimitedStream.js index c206d145b..fb7689d92 100644 --- a/src/builtin-services/private/RateLimitedStream.js +++ b/src/builtin-services/private/RateLimitedStream.js @@ -117,7 +117,10 @@ export class RateLimitedStream { } if (isSocket) { - inner.on('timeout', () => this.#outerStream.emit('timeout')); + inner.on('timeout', () => { + this.#logger?.timedOut(); + this.#outerStream.emit('timeout'); + }); } if (isSocket) { @@ -346,8 +349,8 @@ export class RateLimitedStream { } /** - * @returns {number} The idle-timeout time, in msec. `0` indicates that - * timeout is disabled. + * @returns {number} The idle-timeout time, in msec (`Socket` interface). + * `0` indicates that timeout is disabled. */ get timeout() { return this.#outerThis.#innerStream.timeout; @@ -358,8 +361,33 @@ export class RateLimitedStream { * indicates that timeout is disabled. */ set timeout(timeoutMsec) { + this.setTimeout(timeoutMsec); + } + + /** + * Passthrough of same-named method to the underlying socket. + */ + destroySoon() { + this.#outerThis.#innerStream.destroySoon(); + } + + /** + * Sets a new value for the socket timeout, and optionally adds a `timeout` + * listener. + * + * @param {number} timeoutMsec The new idle-timeout time, in msec. `0` + * indicates that timeout is disabled. + * @param {?function()} [callback = null] Optional callback function. + */ + setTimeout(timeoutMsec, callback = null) { MustBe.number(timeoutMsec, { finite: true, minInclusive: 0 }); - this.#outerThis.#innerStream.timeout = timeoutMsec; + this.#outerThis.#innerStream.setTimeout(timeoutMsec); + + if (callback) { + // Note: The `timeout` event gets plumbed through from the inner socket + // to this instance in `createWrapper()`, above. + this.on('timeout', callback); + } } }; From 5068a356d9a67ca87d2cdd87f109daaf5c8dd523 Mon Sep 17 00:00:00 2001 From: Dan Bornstein Date: Tue, 21 Mar 2023 11:11:24 -0700 Subject: [PATCH 13/17] Fix reference. --- src/network-protocol/private/TcpWrangler.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/network-protocol/private/TcpWrangler.js b/src/network-protocol/private/TcpWrangler.js index 091119fe7..d23f6b8b4 100644 --- a/src/network-protocol/private/TcpWrangler.js +++ b/src/network-protocol/private/TcpWrangler.js @@ -209,7 +209,7 @@ export class TcpWrangler extends ProtocolWrangler { await Promise.race([ closedCond.whenTrue(), - timers.setTimeout(TcpWrangler.SOCKET_TIMEOUT_CLOSE_GRACE_PERIOD_MSEC) + timers.setTimeout(TcpWrangler.#SOCKET_TIMEOUT_CLOSE_GRACE_PERIOD_MSEC) ]); if (socket.destroyed) { @@ -222,7 +222,7 @@ export class TcpWrangler extends ProtocolWrangler { await Promise.race([ closedCond.whenTrue(), - timers.setTimeout(TcpWrangler.SOCKET_TIMEOUT_CLOSE_GRACE_PERIOD_MSEC) + timers.setTimeout(TcpWrangler.#SOCKET_TIMEOUT_CLOSE_GRACE_PERIOD_MSEC) ]); if (socket.destroyed) { From 45a8253ad4427134aa4f5b9ccc2e28ab50de08df Mon Sep 17 00:00:00 2001 From: Dan Bornstein Date: Tue, 21 Mar 2023 11:39:58 -0700 Subject: [PATCH 14/17] Fix `destroy()` and `destroySoon()`. --- .../private/RateLimitedStream.js | 33 ++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/src/builtin-services/private/RateLimitedStream.js b/src/builtin-services/private/RateLimitedStream.js index fb7689d92..5945bae18 100644 --- a/src/builtin-services/private/RateLimitedStream.js +++ b/src/builtin-services/private/RateLimitedStream.js @@ -132,6 +132,19 @@ export class RateLimitedStream { } } + /** + * Processes a `_destroy()` call (indicator that the instance has been + * "destroyed") from the outer stream. + * + * @param {?Error} error Optional error. + * @param {function(Error)} callback Callback to call when this method is + * finished. + */ + #destroy(error, callback) { + this.#innerStream.destroy(error); + callback(); + } + /** * Handles an `error` event from the inner stream. * @@ -305,6 +318,11 @@ export class RateLimitedStream { callback(); } + /** @override */ + _destroy(...args) { + this.#outerThis.#destroy(...args); + } + /** @override */ _read(...args) { this.#outerThis.#read(...args); @@ -368,7 +386,20 @@ export class RateLimitedStream { * Passthrough of same-named method to the underlying socket. */ destroySoon() { - this.#outerThis.#innerStream.destroySoon(); + if (!this.destroyed) { + if (this.closed) { + // This wrapper has already been closed, just not destroyed. The only + // thing to do is destroy it. + this.destroy(); + } else { + // The wrapper hasn't yet been closed, so recapitulate the expected + // behavior from `Socket`, namely to `end()` the stream and then + // `destroy()` it. + this.end(() => { + this.destroy(); + }); + } + } } /** From b73026d203375ead5d042c963a858fb0ab702b15 Mon Sep 17 00:00:00 2001 From: Dan Bornstein Date: Tue, 21 Mar 2023 11:45:08 -0700 Subject: [PATCH 15/17] Use `setTimeout()`. ...because just setting the property doesn't work on `Socket` (though it does on our rate-limiter wrapper). --- src/network-protocol/private/TcpWrangler.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/network-protocol/private/TcpWrangler.js b/src/network-protocol/private/TcpWrangler.js index d23f6b8b4..9e7732afa 100644 --- a/src/network-protocol/private/TcpWrangler.js +++ b/src/network-protocol/private/TcpWrangler.js @@ -146,8 +146,7 @@ export class TcpWrangler extends ProtocolWrangler { // leakage consistent with the issue in this project, and the working // hypothesis is that setting this timeout will suffice as a fix / // workaround (depending on one's perspective). - socket.timeout = TcpWrangler.#SOCKET_TIMEOUT_MSEC; - socket.on('timeout', async () => { + socket.setTimeout(TcpWrangler.#SOCKET_TIMEOUT_MSEC, () => { this.#handleTimeout(socket, connLogger); }); From 9e7318464146b2516b9c500af40556efc85322c2 Mon Sep 17 00:00:00 2001 From: Dan Bornstein Date: Tue, 21 Mar 2023 12:08:06 -0700 Subject: [PATCH 16/17] Work around a bug in `net:http2`. --- src/network-protocol/private/Http2Wrangler.js | 25 ++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/src/network-protocol/private/Http2Wrangler.js b/src/network-protocol/private/Http2Wrangler.js index 0046785f4..139634594 100644 --- a/src/network-protocol/private/Http2Wrangler.js +++ b/src/network-protocol/private/Http2Wrangler.js @@ -116,9 +116,32 @@ export class Http2Wrangler extends TcpWrangler { session.on('frameError', removeSession); session.on('goaway', removeSession); + // What's going on: If the underlying socket was closed and we didn't do + // anything here (that is, if this event handler weren't added), the HTTP2 + // session-handling code wouldn't fully notice by itself. Later, the session + // would do its idle timeout, and the callback here (below) would try to + // close the session. At that point, the HTTP2 system would get confused and + // end up throwing an unhandleable error (method call on the internal socket + // reference, except the reference had gotten `null`ed out). So, with that + // as context, if -- as we do here -- we tell the session to close as soon + // as we see the underlying socket go away, there's no internal HTTP2 error. + ctx.socket.on('close', () => { + if (!session.closed) { + session.close(); + + // When we're in this situation, the HTTP2 library doesn't seem to emit + // the expected `close` event by itself. + session.emit('close'); + } + }); + session.setTimeout(Http2Wrangler.#SESSION_TIMEOUT_MSEC, () => { ctx.sessionLogger?.idleTimeout(); - session.close(); + if (session.closed) { + ctx.sessionLogger?.alreadyClosed(); + } else { + session.close(); + } }); } From 735ffa1ad5ae3e4f5fa774029b01da16df05db28 Mon Sep 17 00:00:00 2001 From: Dan Bornstein Date: Tue, 21 Mar 2023 12:13:09 -0700 Subject: [PATCH 17/17] Note the Node bugs. --- src/network-protocol/private/Http2Wrangler.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/network-protocol/private/Http2Wrangler.js b/src/network-protocol/private/Http2Wrangler.js index 139634594..ea5a2a7ec 100644 --- a/src/network-protocol/private/Http2Wrangler.js +++ b/src/network-protocol/private/Http2Wrangler.js @@ -125,6 +125,9 @@ export class Http2Wrangler extends TcpWrangler { // reference, except the reference had gotten `null`ed out). So, with that // as context, if -- as we do here -- we tell the session to close as soon // as we see the underlying socket go away, there's no internal HTTP2 error. + // Salient issues in Node: + // * + // * ctx.socket.on('close', () => { if (!session.closed) { session.close();