diff --git a/AGENTS.md b/AGENTS.md index 3a7c258..25783ae 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -214,6 +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 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. +- **CHANGELOG.md — what changed for a consumer, per release.** Written for whoever takes the package + off npm, never for the 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 behaviour that changed on the wire. - **AGENTS.md — what may not change, and why.** Hard rules, architecture, conventions, and an index @@ -299,6 +301,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. - 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 new file mode 100644 index 0000000..dd08c4f --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,9 @@ +# 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. diff --git a/README.md b/README.md index f29ff31..fe67fac 100644 --- a/README.md +++ b/README.md @@ -227,6 +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. | | `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 df1797b..9596801 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -524,6 +524,19 @@ 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 + `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. 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.** Maintainer's call, 2026-08-31, narrowed 2026-09-05 via the interop plan: a `command_length` below 16 or above `maxPduLength` leaves nothing that can say where the next PDU starts, so it tears the diff --git a/src/client.ts b/src/client.ts index b935c2f..1f52457 100644 --- a/src/client.ts +++ b/src/client.ts @@ -22,6 +22,7 @@ export type ClientOptions = { addrNpi?: number; addrTon?: number; bindType?: BindType; + connectTimeout?: number; enquireLinkInterval?: number; host?: string; idleTimeout?: number; @@ -52,7 +53,22 @@ const defaults = { username: 'user', } as const; +function armConnectTimeout( + sock: Socket, + connectTimeout: number | undefined, + 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. + return setTimeout(() => { + sock.destroy(); + settle({ err: new Error(`Timed out connecting after ${String(connectTimeout)} ms`) }); + }, connectTimeout).unref(); +} + function openSocket(options: ClientOptions): Promise> { + const connectTimeout = options.connectTimeout; const host = options.host ?? defaults.host; const port = options.port ?? defaults.port; const secure = options.tls !== undefined && options.tls !== false; @@ -78,6 +94,8 @@ function openSocket(options: ClientOptions): Promise> { } const settle = (result: Result<{ sock: Socket }>): void => { + if (timer) clearTimeout(timer); + sock.removeListener('error', onError); signal?.removeEventListener('abort', onAbort); resolve(result); @@ -97,6 +115,8 @@ function openSocket(options: ClientOptions): Promise> { sock.once(secure ? 'secureConnect' : 'connect', () => { settle({ sock }); }); + + const timer = armConnectTimeout(sock, connectTimeout, settle); }); } diff --git a/src/session-options.ts b/src/session-options.ts index c085a22..23aed0f 100644 --- a/src/session-options.ts +++ b/src/session-options.ts @@ -144,14 +144,11 @@ export function checkSessionOptions(options: CheckableOptions): VoidResult { return { err: new Error('fromStart is part of the reconnect policy, spell it reconnect: { fromStart: true }') }; } - const checked = checkLimits([ - ['idleTimeout', options.idleTimeout ?? 0, 0], - ['maxOutstanding', options.maxOutstanding ?? defaults.maxOutstanding, 1], - ['maxReassembly', options.maxReassembly ?? defaults.maxReassembly, 1], - ['reassemblyTimeout', options.reassemblyTimeout ?? defaults.reassemblyTimeout, 0], - ['responseTimeout', options.responseTimeout ?? defaults.responseTimeout, 0], - ['shutdownTimeout', options.shutdownTimeout ?? defaults.shutdownTimeout, 0], - ]); + const connect = checkConnectTimeout(options.connectTimeout); + + if (connect.err) return connect; + + const checked = checkLimits(limitsOf(options)); if (checked.err) return checked; @@ -160,6 +157,28 @@ export function checkSessionOptions(options: CheckableOptions): VoidResult { return backoff.err ? backoff : checkSmsIdFormat(options.smsIdFormat); } +function limitsOf(options: CheckableOptions): [string, number, number][] { + return [ + ['idleTimeout', options.idleTimeout ?? 0, 0], + ['maxOutstanding', options.maxOutstanding ?? defaults.maxOutstanding, 1], + ['maxReassembly', options.maxReassembly ?? defaults.maxReassembly, 1], + ['reassemblyTimeout', options.reassemblyTimeout ?? defaults.reassemblyTimeout, 0], + ['responseTimeout', options.responseTimeout ?? defaults.responseTimeout, 0], + ['shutdownTimeout', options.shutdownTimeout ?? defaults.shutdownTimeout, 0], + ]; +} + +/** Leaving it out is how the wait stays the OS's, so 0 would be a second spelling for that. */ +function checkConnectTimeout(connectTimeout: number | undefined): VoidResult { + if (connectTimeout === undefined) return {}; + + if (Number.isInteger(connectTimeout) && connectTimeout >= 1) return {}; + + const got = String(connectTimeout); + + return { err: new Error(`connectTimeout must be 1 or more, got ${got}; omit it to wait the OS out`) }; +} + function checkLimits(limits: [string, number, number][]): VoidResult { for (const [name, value, min] of limits) { if (!Number.isInteger(value) || value < min) { @@ -238,6 +257,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; /** 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 2667867..eedd257 100644 --- a/test/session-extras.test.ts +++ b/test/session-extras.test.ts @@ -84,6 +84,21 @@ function within(ms: number, promise: Promise): Promise { return Promise.race([promise, delay(ms).then((): undefined => undefined)]); } +/** A client still retrying holds a socket and a timer nothing else releases. */ +function abortAfter( + t: TestContext, + controller: AbortController, + connecting: ReturnType, +): void { + t.after(async () => { + controller.abort(); + + const { session } = await connecting; + + await session?.close({ signal: AbortSignal.abort() }); + }); +} + function submitPdu(seqNr: number, cmdStatus: ErrorName = 'ESME_ROK'): PduObject { return { cmdId: 0x00000004, @@ -798,21 +813,6 @@ describe('reconnect from the first bind', () => { return port; } - /** A client still retrying holds a socket and a timer nothing else releases. */ - function abortAfter( - t: TestContext, - controller: AbortController, - connecting: ReturnType, - ): void { - t.after(async () => { - controller.abort(); - - const { session } = await connecting; - - await session?.close({ signal: AbortSignal.abort() }); - }); - } - test('gives up on the first attempt where reconnect alone is asked for', async () => { const port = await closedPort(); const spy = logSpy(); @@ -1018,6 +1018,86 @@ describe('reconnect from the first bind', () => { }); }); +describe('connectTimeout', () => { + /** 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 }> { + const accepted: net.Socket[] = []; + const listener = net.createServer(sock => { accepted.push(sock); }); + + closeListenerAfter(t, listener, accepted); + await new Promise(resolve => { listener.listen(0, '127.0.0.1', resolve); }); + + const address = listener.address(); + + return { accepted, port: typeof address === 'object' && address !== null ? address.port : 0 }; + } + + test('gives up on a connect the peer never completes', async t => { + const { port } = await stalledListener(t); + const started = Date.now(); + const settled = await within(2000, client({ + connectTimeout: 150, + host: '127.0.0.1', + port, + reconnect: false, + tls: true, + })); + + 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.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 => { + const { accepted, port } = await stalledListener(t); + const controller = new AbortController(); + const connecting = client({ + connectTimeout: 60, + host: '127.0.0.1', + port, + reconnect: { fromStart: true, maxDelay: 40, minDelay: 10 }, + signal: controller.signal, + tls: true, + }); + + abortAfter(t, controller, connecting); + await delay(400); + + assert.ok(accepted.length >= 3, `the loop retried what timed out, got ${String(accepted.length)} attempts`); + }); + + test('disarms on the connect that completed, rather than on the socket that follows it', async t => { + const smpp = await startServer(t); + const { session } = await connect(t, smpp, { connectTimeout: 50 }); + + assert.ok(session); + await delay(150); + + const probe = await session.send({ cmdName: 'enquire_link' }); + + assert.equal(probe.err, undefined); + assert.equal(session.sock.destroyed, false); + }); + + test('refuses a connect timeout that would turn itself off', async () => { + assert.match( + checkSessionOptions({ connectTimeout: 0 }).err?.message ?? '', + /omit 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.5 }).err?.message ?? '', /connectTimeout/); + assert.equal(checkSessionOptions({ connectTimeout: 1000 }).err, undefined); + + const refused = await client({ connectTimeout: 0, port: 1 }); + + assert.ok(refused.err instanceof Error); + assert.equal(refused.session, undefined); + }); +}); + describe('sends across a reconnect', () => { /** Answers every message after the first, which is left to hold the send window open. */ function answerAfterTheFirst(smpp: SmppServer, arrived: string[]): Latch { diff --git a/todo.md b/todo.md index c6f7324..fabfb18 100644 --- a/todo.md +++ b/todo.md @@ -179,16 +179,11 @@ the rewrite, for a dependency added later. Maintainer's call, 2026-09-14. ## Worth doing, not blocking -- [ ] **Give up a connect after `connectTimeout` instead of waiting the OS out.** `openSocket()` in - `client.ts` settles only on `connect`/`secureConnect`, a socket `error`, or the caller's - `signal`, and `reconnect-loop.ts` times nothing but its backoff — so a host that drops SYNs - stalls every attempt for the OS TCP timeout, around 130 s on Linux at the default - `tcp_syn_retries`, and `reconnect.fromStart` cannot retry what never returns. The option aborts - the socket and returns the ordinary connect failure; absent, the wait stays exactly 0.5.0's, so - a call passing no options is unchanged (goal 6). Whether a later major makes it a default is - the open half. Found comparing 0.5.0 with the seven `larvitsmpp` forks, 2026-09-20 — - [SwiftHero/larvitsmpp](https://github.com/SwiftHero/larvitsmpp) added a 10 s connect timeout in - 2017, and it is the one fork change with no equivalent here. +- [ ] **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. - [ ] **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