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
8 changed files with 48 additions and 21 deletions
Showing only changes of commit 8f416c2bc4 - Show all commits
+3 -2
View File
@@ -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, `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. and a field added to that type fails to compile in every copy at once.
- `message_id` values the library generates are UUID v7. - `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 - A socket a test opens and never reads must be `resume()`d, and a `data` listener counts. An unread
peer's FIN, so `server.close()` hangs forever — that is a test bug, not a library one. 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 - 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 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 `node --test` alive until CI's ten-minute cap. `test/teardown.ts` covers a session, a server and a
+8 -5
View File
@@ -1,9 +1,12 @@
# Changelog # Changelog
What changed for someone using `@larvit/smpp`, newest first.
## Unreleased ## Unreleased
- `client()` takes `connectTimeout`, which gives up on a connect the SMSC never completes — the TLS - `client()` takes `connectTimeout`, which gives up on a connect attempt the SMSC never completes —
handshake included — and reports it as an ordinary connect failure, so `reconnect` retries it on the TLS handshake included — and reports it as an ordinary connect failure, so `reconnect` retries
its usual backoff. Leave it out and the wait is the operating system's, as before. 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).
+5 -1
View File
@@ -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. | | `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. | | `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. | | `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. | | `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`. | | `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. | | `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. | | 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`. | | 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 ## Migrating from larvitsmpp 0.4.0
See [MIGRATION.md](https://gitea.larvit.se/larvit/smpp-js/src/branch/main/MIGRATION.md). See [MIGRATION.md](https://gitea.larvit.se/larvit/smpp-js/src/branch/main/MIGRATION.md).
+8 -3
View File
@@ -56,14 +56,19 @@ const defaults = {
function armConnectTimeout( function armConnectTimeout(
sock: Socket, sock: Socket,
connectTimeout: number | undefined, connectTimeout: number | undefined,
peer: string,
settle: (result: Result<{ sock: Socket }>) => void, settle: (result: Result<{ sock: Socket }>) => void,
): NodeJS.Timeout | undefined { ): NodeJS.Timeout | undefined {
if (connectTimeout === undefined) return 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(() => { return setTimeout(() => {
sock.destroy(); 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(); }, connectTimeout).unref();
} }
@@ -115,7 +120,7 @@ function openSocket(options: ClientOptions): Promise<Result<{ sock: Socket }>> {
settle({ sock }); settle({ sock });
}); });
const timer = armConnectTimeout(sock, connectTimeout, settle); const timer = armConnectTimeout(sock, connectTimeout, `${host}:${String(port)}`, settle);
}); });
} }
+6 -7
View File
@@ -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; const maxTimerDelay = 2_147_483_647;
function checkConnectTimeout(connectTimeout: number | undefined): VoidResult { function checkConnectTimeout(connectTimeout: unknown): VoidResult {
if (connectTimeout === undefined) return {}; if (connectTimeout === undefined) return {};
const got = String(connectTimeout); const got = typeof connectTimeout === 'string' ? `"${connectTimeout}"` : namedValue(connectTimeout);
if (!Number.isInteger(connectTimeout) || connectTimeout < 1) { 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 to wait the OS out`) }; 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) { 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 {}; 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. */ /** What the checker reads, as it arrives: a caller without types can put anything in it. */
export type CheckableOptions = { 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. */ /** Not an option: the one spelling is inside reconnect, and this is where the other is refused. */
fromStart?: unknown; fromStart?: unknown;
idleTimeout?: number | undefined; idleTimeout?: number | undefined;
+11 -2
View File
@@ -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, 'a handshake nothing answers is what the OS wait would swallow for minutes');
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,
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); assert.equal(settled.session, undefined);
}); });
@@ -1090,9 +1094,14 @@ describe('connectTimeout', () => {
); );
assert.match(checkSessionOptions({ connectTimeout: -1 }).err?.message ?? '', /connectTimeout/); assert.match(checkSessionOptions({ connectTimeout: -1 }).err?.message ?? '', /connectTimeout/);
assert.match(checkSessionOptions({ connectTimeout: 1.5 }).err?.message ?? '', /whole number/); 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( assert.match(
checkSessionOptions({ connectTimeout: 2_147_483_648 }).err?.message ?? '', 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', '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: 2_147_483_647 }).err, undefined);
-1
View File
@@ -1493,7 +1493,6 @@ describe('robustness', () => {
// 0.4.0 registered a listener per sequence number and waited forever, leaking one per call. // 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 => { test('gives up on a peer that never answers', async t => {
const accepted: net.Socket[] = []; 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(); }); const silent = net.createServer(sock => { accepted.push(sock); sock.resume(); });
await new Promise<void>(resolve => silent.listen(0, resolve)); await new Promise<void>(resolve => silent.listen(0, resolve));
+7
View File
@@ -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 unit until 1.0.0. Needs a number an operator would recognise, where SwiftHero's fork picked
10 s. 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`, - [ ] **Refuse a delay Node's timers cannot hold, in `checkLimits`.** `idleTimeout`,
`reassemblyTimeout`, `responseTimeout` and `shutdownTimeout` take any integer, and `setTimeout` `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 fires after 1 ms for anything above 2147483647 — so a value in the wrong unit gets the inverse