From 1416615ca35b6b5af00762a45837e36400b45e9c Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Sun, 20 Sep 2026 20:34:53 +0200 Subject: [PATCH] Bound every connect attempt at 10 s by default, with false the way to opt out --- AGENTS.md | 4 ++-- CHANGELOG.md | 7 ++++--- README.md | 2 +- docs/decisions.md | 30 ++++++++++++++++++------------ src/client.ts | 11 ++++++----- src/session-options.ts | 4 ++-- test/session-extras.test.ts | 15 +++++++++++---- todo.md | 6 ------ 8 files changed, 44 insertions(+), 35 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index c68ebfc..42b42d0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -302,8 +302,8 @@ the file. - Coming up is not proof a link works, so only one that outlasted `maxDelay` resets the backoff. - `reconnect: { fromStart: true }` puts the first connect and bind through that same loop, and `client()` then resolves only once it is bound. -- `connectTimeout` is absent by default, bounds the whole connect including the TLS handshake, and - `0` is refused. +- `connectTimeout` defaults to 10 s, bounds the whole connect including the TLS handshake, and + `false` is the one way to turn it off. - A stream this library cannot frame is a dead link; one PDU it cannot parse is not. - A deliberate shutdown drains; an unusable link and an abort do not. - Every segment of a concatenated message is answered as it arrives, so `sendResp()` on one is the diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e6c789..0eee28d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,9 +2,10 @@ ## Unreleased -- `client()` takes `connectTimeout`, which gives up on a connect attempt the SMSC never completes — - the TLS handshake included — and reports it as an ordinary connect failure, so `reconnect` retries - it on its usual backoff. Leave it out and each attempt waits the operating system out, as before. +- `client()` now bounds each connect attempt at 10 seconds, the TLS handshake included, and reports + one that expires as an ordinary connect failure, so `reconnect` retries it on its usual backoff. + A connect previously waited the operating system out, around 130 s on Linux against a host that + drops SYNs. `connectTimeout` retunes the bound, and `connectTimeout: false` restores the old wait. ## 0.5.0 diff --git a/README.md b/README.md index a618b30..eb66a1d 100644 --- a/README.md +++ b/README.md @@ -227,7 +227,7 @@ All optional. Timeouts and delays are milliseconds. | `interfaceVersion` | `0x34` | The SMPP version declared at bind. `0x50` for an SMSC that requires SMPP 5.0. | | `systemType`, `addressRange`, `addrTon`, `addrNpi` | `''`, `''`, `0`, `0` | The remaining bind fields, for operators that require them. | | `tls` | `false` | `true` for defaults, or a `tls.ConnectionOptions` object for a private CA or a client certificate. | -| `connectTimeout` | — | Give up on **each connect attempt** the SMSC never completes, the TLS handshake included, and report it as an ordinary connect failure, which `reconnect` then retries. It bounds one attempt, never the `client()` call. Absent, each attempt waits the operating system out, around 130 s on Linux. | +| `connectTimeout` | `10000` | Give up on **each connect attempt** the SMSC never completes, the TLS handshake included, and report it as an ordinary connect failure, which `reconnect` then retries. It bounds one attempt, never the `client()` call. `false` waits the operating system out instead, around 130 s on Linux. | | `enquireLinkInterval` | `20000` | Interval between `enquire_link` on a quiet link. | | `idleTimeout` | `2 × enquireLinkInterval` | Give up on a link the peer has stopped answering, and re-bind unless `reconnect` is `false`. | | `responseTimeout` | `30000` | How long to wait for a response, and how long a send with no link waits for the next one. `0` waits forever. | diff --git a/docs/decisions.md b/docs/decisions.md index b2bfec7..05cce6a 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -524,19 +524,25 @@ rule and an index of the titles below. `LinkGate`'s hold is not — it is awaited with no other handle, so a process whose only work is `client()` would exit unbound. -- **`connectTimeout` is absent by default, bounds the whole connect including the TLS handshake, and - `0` is refused.** Maintainer's call, 2026-09-20, serving goal 6: a connect that never returns is - one `reconnect` cannot retry, because the operating system holds the attempt for around 130 s at - Linux's default `tcp_syn_retries` and nothing above it is counting. Absent is how that wait stays - the operating system's, so a call passing no options is unchanged, and `0` would be a second - spelling for absent — refused at the call, naming the spelling that turns it off. What expires is - reported as the ordinary connect failure, so the loop retries it like any other. It settles on +- **`connectTimeout` defaults to 10 s, bounds the whole connect including the TLS handshake, and + `false` is the one way to turn it off.** Maintainer's call, 2026-09-20, serving goal 5: a connect + that never returns is one `reconnect` cannot retry, because the operating system holds the attempt + for around 130 s at Linux's default `tcp_syn_retries` and nothing above it is counting — and no + application chooses that, so it is a default rather than an option that switches on what the caller + obviously wanted. Accepted: the far end sees roughly four times the SYNs against a dead host, one + per ~40 s rather than one per ~160 s, which goal 4 tolerates because the backoff still caps the + rate. `false` spells the operating system's wait, as it does for `reconnect`, and `0` is refused + naming it, so one spelling reaches each result. What expires is reported as the ordinary connect + failure, so the loop retries it like any other, and the message names the peer and whether the TCP + connect or the TLS handshake stalled — different faults, different answers. It settles on `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 - 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. `server()` shares the checker - 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. + way a black-holed SYN is. Rejected: shipping the option with no default, which left goal 5's "an + option does not switch on the thing the caller obviously wanted" unmet, and would have cost a + second breaking minor plus a reversal of the `0` spelling to correct later. 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 answer on a link that already exists and + would retune both at once. `server()` shares the checker and ignores the option, as it already + ignores `reconnect` — nothing at that end connects out. - **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 diff --git a/src/client.ts b/src/client.ts index 48f6d0f..ab61ff9 100644 --- a/src/client.ts +++ b/src/client.ts @@ -22,7 +22,7 @@ export type ClientOptions = { addrNpi?: number; addrTon?: number; bindType?: BindType; - connectTimeout?: number; + connectTimeout?: number | false; enquireLinkInterval?: number; host?: string; idleTimeout?: number; @@ -41,8 +41,9 @@ export type ClientOptions = { username?: string; }; -const defaults = { +export const defaults = { bindType: 'transceiver', + connectTimeout: 10_000, enquireLinkInterval: 20_000, host: 'localhost', /** The idle timeout is what notices a dead link, so it has to outlast one silent probe. */ @@ -55,11 +56,11 @@ const defaults = { function armConnectTimeout( sock: Socket, - connectTimeout: number | undefined, + connectTimeout: number | false, peer: string, settle: (result: Result<{ sock: Socket }>) => void, ): NodeJS.Timeout | undefined { - if (connectTimeout === undefined) return undefined; + if (connectTimeout === false) return undefined; // A firewall and a stalled handshake need different answers, and only the phase tells them apart. let phase = `connecting to ${peer}`; @@ -73,7 +74,7 @@ function armConnectTimeout( } function openSocket(options: ClientOptions): Promise> { - const connectTimeout = options.connectTimeout; + const connectTimeout = options.connectTimeout ?? defaults.connectTimeout; const host = options.host ?? defaults.host; const port = options.port ?? defaults.port; const secure = options.tls !== undefined && options.tls !== false; diff --git a/src/session-options.ts b/src/session-options.ts index 0d289ee..01b7372 100644 --- a/src/session-options.ts +++ b/src/session-options.ts @@ -171,12 +171,12 @@ function limitsOf(options: CheckableOptions): [string, number, number][] { const maxTimerDelay = 2_147_483_647; function checkConnectTimeout(connectTimeout: unknown): VoidResult { - if (connectTimeout === undefined) return {}; + if (connectTimeout === undefined || connectTimeout === false) return {}; const got = typeof connectTimeout === 'string' ? `"${connectTimeout}"` : namedValue(connectTimeout); if (typeof connectTimeout !== 'number' || !Number.isInteger(connectTimeout) || connectTimeout < 1) { - return { err: new Error(`connectTimeout must be a whole number of milliseconds, 1 or more, got ${got}; omit it or pass undefined to wait the OS out`) }; + return { err: new Error(`connectTimeout must be a whole number of milliseconds, 1 or more, got ${got}; false waits the OS out instead`) }; } if (connectTimeout > maxTimerDelay) { diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index f9a2d62..2f54f77 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -25,7 +25,7 @@ import { DlrMerger } from '../src/dlr-merger.ts'; import { PduRefusedError } from '../src/pdu-refusal.ts'; import { objToPdu } from '../src/pdu.ts'; import { checkSessionOptions, standsInFor } from '../src/session-options.ts'; -import { client } from '../src/client.ts'; +import { client, defaults as clientDefaults } from '../src/client.ts'; import { closeAfter, closeListenerAfter } from './teardown.ts'; import { concatOf } from '../src/concat.ts'; import { consts } from '../src/defs/constants.ts'; @@ -1087,11 +1087,13 @@ describe('connectTimeout', () => { }); test('refuses a connect timeout that would turn itself off', async () => { + assert.equal(clientDefaults.connectTimeout, 10_000, 'the number the README documents'); assert.match( checkSessionOptions({ connectTimeout: 0 }).err?.message ?? '', - /omit it/, - 'off is spelled by leaving it out, so 0 may not stand in for it', + /false waits the OS out/, + 'off is spelled false, so 0 may not stand in for it', ); + assert.equal(checkSessionOptions({ connectTimeout: false }).err, undefined); assert.match(checkSessionOptions({ connectTimeout: -1 }).err?.message ?? '', /connectTimeout/); assert.match(checkSessionOptions({ connectTimeout: 1.5 }).err?.message ?? '', /whole number/); assert.match( @@ -1110,8 +1112,13 @@ describe('connectTimeout', () => { const refused = await client({ connectTimeout: 0, port: 1 }); 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.match(refused.err.message, /false waits/, 'the socket may not be opened before the option is refused'); assert.equal(refused.session, undefined); + + const off = await client({ connectTimeout: false, port: 1 }); + + assert.ok(off.err instanceof Error); + assert.match(off.err.message, /ECONNREFUSED/, 'false opts out of the bound without breaking the connect'); }); }); diff --git a/todo.md b/todo.md index 586a10b..3de7bbf 100644 --- a/todo.md +++ b/todo.md @@ -179,12 +179,6 @@ the rewrite, for a dependency added later. Maintainer's call, 2026-09-14. ## Worth doing, not blocking -- [ ] **Decide whether a later major gives `connectTimeout` a default.** The option ships absent, so - a call passing none waits the OS out as 0.5.0 did — the half the option deliberately left open. - A default is a breaking change for anyone relying on that wait, and the minor is the breaking - unit until 1.0.0. Needs a number an operator would recognise, where SwiftHero's fork picked - 10 s. - - [ ] **Cut the three teardown sentences `test/teardown.ts` already says.** Under AGENTS.md's Conventions, "`test/teardown.ts` covers a session, a server and a listener" restates its two exported names, "Its close aborts rather than drains" restates `closeAfter`'s own doc comment,