Give up a connect after connectTimeout instead of waiting the OS out #12

Merged
lilleman merged 7 commits from connect-timeout into main 2026-09-20 20:58:14 +02:00
6 changed files with 38 additions and 16 deletions
Showing only changes of commit 84174b4a5b - Show all commits
+2 -2
View File
@@ -214,8 +214,8 @@ Each file answers one question, and a fact belongs to the file whose question it
- **README.md — what you can rely on, and where this is heading.** Observable behaviour, for - **README.md — what you can rely on, and where this is heading.** Observable behaviour, for
someone using the package, plus the goals and the audience. It carries a reason only where the someone using the package, plus the goals and the audience. It carries a reason only where the
reason changes how you would call the thing. reason changes how you would call the thing.
- **CHANGELOG.md — what changed for a consumer, per release.** Written for whoever takes the package - **CHANGELOG.md — what changed for a consumer, per release.** Written for the public, never for the
off npm, never for the next agent, and a line lands there as the work ships rather than at release. next agent, and a line lands there as the work ships rather than at release.
- **MIGRATION.md — what a 0.4.0 consumer has to change.** Renamed and removed surface, and the - **MIGRATION.md — what a 0.4.0 consumer has to change.** Renamed and removed surface, and the
behaviour that changed on the wire. behaviour that changed on the wire.
- **AGENTS.md — what may not change, and why.** Hard rules, architecture, conventions, and an index - **AGENTS.md — what may not change, and why.** Hard rules, architecture, conventions, and an index
+3 -2
View File
@@ -534,8 +534,9 @@ rule and an index of the titles below.
`secureConnect` for a TLS socket, so a peer that accepts and then says nothing is bounded the same `secureConnect` for a TLS socket, so a peer that accepts and then says nothing is bounded the same
way a black-holed SYN is. Rejected: `socket.setTimeout()`, an idle timeout that goes on arming way a black-holed SYN is. Rejected: `socket.setTimeout()`, an idle timeout that goes on arming
once the link is up. Rejected: bounding it with `responseTimeout`, which names the wait for an once the link is up. Rejected: bounding it with `responseTimeout`, which names the wait for an
answer on a link that already exists and would retune both at once. Open while it has no default: answer on a link that already exists and would retune both at once. `server()` shares the checker
whether a later major gives it one. and ignores the option, as it already ignores `reconnect` — nothing at that end connects out. Open
while it has no default: whether a later major gives it one.
- **A stream this library cannot frame is a dead link; one PDU it cannot parse is not.** - **A stream this library cannot frame is a dead link; one PDU it cannot parse is not.**
Maintainer's call, 2026-08-31, narrowed 2026-09-05 via the interop plan: a `command_length` below Maintainer's call, 2026-08-31, narrowed 2026-09-05 via the interop plan: a `command_length` below
+1 -2
View File
@@ -94,8 +94,7 @@ function openSocket(options: ClientOptions): Promise<Result<{ sock: Socket }>> {
} }
const settle = (result: Result<{ sock: Socket }>): void => { const settle = (result: Result<{ sock: Socket }>): void => {
if (timer) clearTimeout(timer); clearTimeout(timer);
sock.removeListener('error', onError); sock.removeListener('error', onError);
signal?.removeEventListener('abort', onAbort); signal?.removeEventListener('abort', onAbort);
resolve(result); resolve(result);
+12 -4
View File
@@ -168,15 +168,23 @@ function limitsOf(options: CheckableOptions): [string, number, number][] {
]; ];
} }
/** Leaving it out is how the wait stays the OS's, so 0 would be a second spelling for that. */ /** Node's timers are 32-bit, and a delay above this one fires after 1 ms instead of never. */
const maxTimerDelay = 2_147_483_647;
function checkConnectTimeout(connectTimeout: number | undefined): VoidResult { function checkConnectTimeout(connectTimeout: number | undefined): VoidResult {
if (connectTimeout === undefined) return {}; if (connectTimeout === undefined) return {};
if (Number.isInteger(connectTimeout) && connectTimeout >= 1) return {};
const got = String(connectTimeout); const got = String(connectTimeout);
return { err: new Error(`connectTimeout must be 1 or more, got ${got}; omit it to wait the OS out`) }; if (!Number.isInteger(connectTimeout) || connectTimeout < 1) {
return { err: new Error(`connectTimeout must be a whole number of milliseconds, 1 or more, got ${got}; omit it to wait the OS out`) };
}
if (connectTimeout > maxTimerDelay) {
return { err: new Error(`connectTimeout must be ${String(maxTimerDelay)} or less, got ${got}`) };
}
return {};
} }
function checkLimits(limits: [string, number, number][]): VoidResult { function checkLimits(limits: [string, number, number][]): VoidResult {
+14 -6
View File
@@ -1022,7 +1022,10 @@ describe('connectTimeout', () => {
/** Accepts and then says nothing, so a TLS handshake started on it never completes. */ /** Accepts and then says nothing, so a TLS handshake started on it never completes. */
async function stalledListener(t: TestContext): Promise<{ accepted: net.Socket[]; port: number }> { async function stalledListener(t: TestContext): Promise<{ accepted: net.Socket[]; port: number }> {
const accepted: net.Socket[] = []; const accepted: net.Socket[] = [];
const listener = net.createServer(sock => { accepted.push(sock); }); const listener = net.createServer(sock => {
accepted.push(sock);
sock.resume();
});
closeListenerAfter(t, listener, accepted); closeListenerAfter(t, listener, accepted);
await new Promise<void>(resolve => { listener.listen(0, '127.0.0.1', resolve); }); await new Promise<void>(resolve => { listener.listen(0, '127.0.0.1', resolve); });
@@ -1034,7 +1037,6 @@ describe('connectTimeout', () => {
test('gives up on a connect the peer never completes', async t => { test('gives up on a connect the peer never completes', async t => {
const { port } = await stalledListener(t); const { port } = await stalledListener(t);
const started = Date.now();
const settled = await within(2000, client({ const settled = await within(2000, client({
connectTimeout: 150, connectTimeout: 150,
host: '127.0.0.1', host: '127.0.0.1',
@@ -1047,7 +1049,6 @@ describe('connectTimeout', () => {
assert.ok(settled.err instanceof Error); assert.ok(settled.err instanceof Error);
assert.match(settled.err.message, /Timed out connecting/); assert.match(settled.err.message, /Timed out connecting/);
assert.equal(settled.session, undefined); assert.equal(settled.session, undefined);
assert.ok(Date.now() - started >= 150, 'the timeout is what settles it, not a socket error');
}); });
test('retries a connect it timed out on, like any other failed attempt', async t => { test('retries a connect it timed out on, like any other failed attempt', async t => {
@@ -1070,10 +1071,10 @@ describe('connectTimeout', () => {
test('disarms on the connect that completed, rather than on the socket that follows it', async t => { test('disarms on the connect that completed, rather than on the socket that follows it', async t => {
const smpp = await startServer(t); const smpp = await startServer(t);
const { session } = await connect(t, smpp, { connectTimeout: 50 }); const { session } = await connect(t, smpp, { connectTimeout: 200 });
assert.ok(session); assert.ok(session);
await delay(150); await delay(300);
const probe = await session.send({ cmdName: 'enquire_link' }); const probe = await session.send({ cmdName: 'enquire_link' });
@@ -1088,12 +1089,19 @@ describe('connectTimeout', () => {
'off is spelled by leaving it out, so 0 may not stand in for it', 'off is spelled by leaving it out, so 0 may not stand in for it',
); );
assert.match(checkSessionOptions({ connectTimeout: -1 }).err?.message ?? '', /connectTimeout/); assert.match(checkSessionOptions({ connectTimeout: -1 }).err?.message ?? '', /connectTimeout/);
assert.match(checkSessionOptions({ connectTimeout: 1.5 }).err?.message ?? '', /connectTimeout/); assert.match(checkSessionOptions({ connectTimeout: 1.5 }).err?.message ?? '', /whole number/);
assert.match(
checkSessionOptions({ connectTimeout: 2_147_483_648 }).err?.message ?? '',
/2147483647 or less/,
'a delay Node cannot hold in 32 bits fires after 1 ms, the inverse of what it asked for',
);
assert.equal(checkSessionOptions({ connectTimeout: 2_147_483_647 }).err, undefined);
assert.equal(checkSessionOptions({ connectTimeout: 1000 }).err, undefined); assert.equal(checkSessionOptions({ connectTimeout: 1000 }).err, undefined);
const refused = await client({ connectTimeout: 0, port: 1 }); const refused = await client({ connectTimeout: 0, port: 1 });
assert.ok(refused.err instanceof Error); assert.ok(refused.err instanceof Error);
assert.match(refused.err.message, /omit it/, 'the socket may not be opened before the option is refused');
assert.equal(refused.session, undefined); assert.equal(refused.session, undefined);
}); });
}); });
+6
View File
@@ -185,6 +185,12 @@ the rewrite, for a dependency added later. Maintainer's call, 2026-09-14.
unit until 1.0.0. Needs a number an operator would recognise, where SwiftHero's fork picked unit until 1.0.0. Needs a number an operator would recognise, where SwiftHero's fork picked
10 s. 10 s.
- [ ] **Refuse a delay Node's timers cannot hold, in `checkLimits`.** `idleTimeout`,
`reassemblyTimeout`, `responseTimeout` and `shutdownTimeout` take any integer, and `setTimeout`
fires after 1 ms for anything above 2147483647 — so a value in the wrong unit gets the inverse
of what it asked for, explained only by a warning on stderr. `connectTimeout` refuses one
already, which is the asymmetry to close. Raised by review, 2026-09-20.
- [ ] **A send the codec will refuse waits for a link and a window slot first.** `refuse()` in - [ ] **A send the codec will refuse waits for a link and a window slot first.** `refuse()` in
`outgoing-requests.ts` runs `misuse()` and the abort check before the wait, precisely so a call `outgoing-requests.ts` runs `misuse()` and the abort check before the wait, precisely so a call
that can never go out does not queue for what it will never use; a body `objToPdu()` refuses on that can never go out does not queue for what it will never use; a body `objToPdu()` refuses on