From 8f416c2bc413b5f05db433e51293c712e4fb4c93 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Sun, 20 Sep 2026 18:17:39 +0200 Subject: [PATCH] Name the peer and the stalled phase on a connect timeout, and quote an untyped value --- AGENTS.md | 5 +++-- CHANGELOG.md | 13 ++++++++----- README.md | 6 +++++- src/client.ts | 11 ++++++++--- src/session-options.ts | 13 ++++++------- test/session-extras.test.ts | 13 +++++++++++-- test/session.test.ts | 1 - todo.md | 7 +++++++ 8 files changed, 48 insertions(+), 21 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 1f0829e..c68ebfc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -193,8 +193,9 @@ decision under [The wire](docs/decisions.md#the-wire). `message-class.test.ts` and `unsendable.test.ts` is one `SendSmsDeps.send` that answers nothing, and a field added to that type fails to compile in every copy at once. - `message_id` values the library generates are UUID v7. -- A test that needs a dummy peer must `resume()` its sockets. An unread socket never processes the - peer's FIN, so `server.close()` hangs forever — that is a test bug, not a library one. +- A socket a test opens and never reads must be `resume()`d, and a `data` listener counts. An unread + socket never processes the peer's FIN, so `server.close()` hangs forever — that is a test bug, not + a library one. - Everything a test opens gets its teardown registered as it is opened, never closed on the test's last line: an assertion that throws skips that line, and the listener it leaves behind keeps `node --test` alive until CI's ten-minute cap. `test/teardown.ts` covers a session, a server and a diff --git a/CHANGELOG.md b/CHANGELOG.md index dd08c4f..4e6c789 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,9 +1,12 @@ # Changelog -What changed for someone using `@larvit/smpp`, newest first. - ## Unreleased -- `client()` takes `connectTimeout`, which gives up on a connect 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 the wait is the operating system's, as before. +- `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. + +## 0.5.0 + +The TypeScript rewrite. What a 0.4.0 consumer has to change is in +[MIGRATION.md](https://gitea.larvit.se/larvit/smpp-js/src/branch/main/MIGRATION.md). diff --git a/README.md b/README.md index fe67fac..a618b30 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 a connect the SMSC never completes, the TLS handshake included, and report it as an ordinary connect failure, which `reconnect` then retries. Absent, the wait is the operating system's, around 130 s on Linux. | +| `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. | | `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. | @@ -630,6 +630,10 @@ if (isCommand(pduObj, 'submit_sm')) { | Spec tables | `cmds`, `consts`, `encodings`, `errors`, `tlvs`, `types`, the `cmdsById`, `constsById`, `errorsById` and `tlvsById` maps, and all of them grouped as `defs`. `isCommandName`, `isErrorName`, `isEncodingName`, `commandNameById` and `errorNameById` narrow a value into them. | | Types | Every option, result, event payload and table entry has a named type: `ClientOptions`, `ServerOptions`, `SendSmsOptions`, `SendSmsResult`, `Sms`, `Dlr`, `MessageDlr`, `Receipt`, `PduObject`, `PduHeader`, `SmppLog`, `Result` and the rest in `dist/index.d.ts`. | +## What changed per release + +See [CHANGELOG.md](https://gitea.larvit.se/larvit/smpp-js/src/branch/main/CHANGELOG.md). + ## Migrating from larvitsmpp 0.4.0 See [MIGRATION.md](https://gitea.larvit.se/larvit/smpp-js/src/branch/main/MIGRATION.md). diff --git a/src/client.ts b/src/client.ts index 0894270..48f6d0f 100644 --- a/src/client.ts +++ b/src/client.ts @@ -56,14 +56,19 @@ const defaults = { function armConnectTimeout( sock: Socket, connectTimeout: number | undefined, + peer: string, settle: (result: Result<{ sock: Socket }>) => void, ): NodeJS.Timeout | undefined { if (connectTimeout === undefined) return undefined; - // The connecting socket is what holds the process, so this wait never has to. + // A firewall and a stalled handshake need different answers, and only the phase tells them apart. + let phase = `connecting to ${peer}`; + + sock.once('connect', () => { phase = `completing the TLS handshake with ${peer}`; }); + return setTimeout(() => { sock.destroy(); - settle({ err: new Error(`Timed out connecting after ${String(connectTimeout)} ms`) }); + settle({ err: new Error(`Timed out ${phase} after ${String(connectTimeout)} ms`) }); }, connectTimeout).unref(); } @@ -115,7 +120,7 @@ function openSocket(options: ClientOptions): Promise> { settle({ sock }); }); - const timer = armConnectTimeout(sock, connectTimeout, settle); + const timer = armConnectTimeout(sock, connectTimeout, `${host}:${String(port)}`, settle); }); } diff --git a/src/session-options.ts b/src/session-options.ts index 46592c4..0d289ee 100644 --- a/src/session-options.ts +++ b/src/session-options.ts @@ -168,20 +168,19 @@ function limitsOf(options: CheckableOptions): [string, number, number][] { ]; } -/** 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: unknown): VoidResult { if (connectTimeout === undefined) return {}; - const got = String(connectTimeout); + const got = typeof connectTimeout === 'string' ? `"${connectTimeout}"` : namedValue(connectTimeout); - 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 (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`) }; } if (connectTimeout > maxTimerDelay) { - return { err: new Error(`connectTimeout must be ${String(maxTimerDelay)} or less, got ${got}`) }; + return { err: new Error(`connectTimeout must be ${String(maxTimerDelay)} ms or less (about 24 days), got ${got}`) }; } return {}; @@ -265,7 +264,7 @@ function checkSmsIdFormat(smsIdFormat: unknown): VoidResult { /** What the checker reads, as it arrives: a caller without types can put anything in it. */ export type CheckableOptions = { - connectTimeout?: number | undefined; + connectTimeout?: unknown; /** Not an option: the one spelling is inside reconnect, and this is where the other is refused. */ fromStart?: unknown; idleTimeout?: number | undefined; diff --git a/test/session-extras.test.ts b/test/session-extras.test.ts index 00cc60f..f9a2d62 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -1047,7 +1047,11 @@ describe('connectTimeout', () => { assert.ok(settled, 'a handshake nothing answers is what the OS wait would swallow for minutes'); assert.ok(settled.err instanceof Error); - assert.match(settled.err.message, /Timed out connecting/); + assert.match( + settled.err.message, + new RegExp(`Timed out completing the TLS handshake with 127\\.0\\.0\\.1:${String(port)} after 150 ms`), + 'a firewall and a peer that accepts then stalls need different answers from the operator', + ); assert.equal(settled.session, undefined); }); @@ -1090,9 +1094,14 @@ describe('connectTimeout', () => { ); assert.match(checkSessionOptions({ connectTimeout: -1 }).err?.message ?? '', /connectTimeout/); assert.match(checkSessionOptions({ connectTimeout: 1.5 }).err?.message ?? '', /whole number/); + assert.match( + checkSessionOptions({ connectTimeout: '5000' }).err?.message ?? '', + /got "5000"/, + 'an env var read without Number() is the commonest untyped value, and it is a correct number', + ); assert.match( checkSessionOptions({ connectTimeout: 2_147_483_648 }).err?.message ?? '', - /2147483647 or less/, + /2147483647 ms 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); diff --git a/test/session.test.ts b/test/session.test.ts index db52bc8..fa7d0a1 100644 --- a/test/session.test.ts +++ b/test/session.test.ts @@ -1493,7 +1493,6 @@ describe('robustness', () => { // 0.4.0 registered a listener per sequence number and waited forever, leaking one per call. test('gives up on a peer that never answers', async t => { const accepted: net.Socket[] = []; - // resume() so the socket drains; an unread socket never notices the peer hanging up. const silent = net.createServer(sock => { accepted.push(sock); sock.resume(); }); await new Promise(resolve => silent.listen(0, resolve)); diff --git a/todo.md b/todo.md index 00875e1..586a10b 100644 --- a/todo.md +++ b/todo.md @@ -185,6 +185,13 @@ 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 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, + and the `net.Server.close()` sentence restates `closeListenerAfter`'s. Keep the registered-at- + creation rule and the FIFO one, which nothing else states, and drop "CI's ten-minute cap" — + that number lives in `.gitea/workflows/test.yaml`. Raised by the prose pass, 2026-09-20. + - [ ] **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