From 0e8f2989576e29f0590d2126bab2a102acf491d0 Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Thu, 27 Aug 2026 22:22:51 +0200 Subject: [PATCH] Route a rejecting async listener to sessionError instead of the process --- AGENTS.md | 8 +++++ README.md | 2 +- eslint.config.js | 4 +-- src/server.ts | 14 ++++++++- src/session.ts | 14 ++++++++- test/session.test.ts | 73 +++++++++++++++++++++++++++++++++++++++++++- todo.md | 12 ++------ 7 files changed, 111 insertions(+), 16 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 17d0a0f..9db9dac 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -213,6 +213,14 @@ exactly 140. 0x80-0xFF for MC-vendor-specific values, so an unnameable one keeps its raw `statusId` and leaves `statusMsg` to the body. +- **A listener that rejects is routed by Node's `captureRejections`, not by hand-dispatching.** Both + emitters construct with `captureRejections: true` and implement + `[EventEmitter.captureRejectionSymbol]`, which lands a rejected `async` listener on `sessionError` + or `serverError` beside the synchronous guard in `emit()`. Dispatching `rawListeners()` from + `emit()` instead needs a cast to call them with the event's argument tuple, which hard rule 4 + forbids. `Session.on()` still types its listeners as void-returning, because widening that needs + the same cast — which is why `test/*.test.ts` turns `no-misused-promises` off. + - **The TLS tests build their own self-signed certificate in DER** (`test/tls.test.ts`) instead of adding a devDependency or shelling out to openssl. Maintainer's call, 2026-08-26: the dev image `node:24.18.0-bookworm-slim` ships no openssl binary, so a shelled-out fixture would pass in CI diff --git a/README.md b/README.md index e4c47f3..44304ff 100644 --- a/README.md +++ b/README.md @@ -300,7 +300,7 @@ TypeScript users can import `SmppLog` to have the compiler check one. | `messageDlr` | Every segment of a multipart message sent with `dlr: true` has been reported on, carrying the worst status of the segments. Merging needs the SMSC to number its segment ids `-`, which is this library's own server's convention — an SMSC that hands out unrelated ids per segment never fires it. | | `close` | The connection closed. | | `reconnected` | The client re-bound after a drop (only with `reconnect` configured). | -| `sessionError` | Something failed on a live session, including a hook or listener that threw. | +| `sessionError` | Something failed on a live session, including a hook or listener that threw or, if it was `async`, rejected. | | `data` | Raw bytes arrived on the socket. | | `incomingPdu` | A complete PDU arrived, as a buffer. | | `incomingPduObj` | The same PDU, parsed into an object. | diff --git a/eslint.config.js b/eslint.config.js index 307e09d..c8e8cfa 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -49,8 +49,8 @@ export default tseslint.config( rules: { 'no-control-regex': 'off' }, }, { - // Mirrors the README's examples as written; an async event listener is part of what they show. - files: ['test/readme.test.ts'], + // An async event listener is a documented, supported shape; EventEmitter types listeners void. + files: ['test/*.test.ts'], rules: { '@typescript-eslint/no-misused-promises': 'off' }, }, { diff --git a/src/server.ts b/src/server.ts index 8352442..b1e52a6 100644 --- a/src/server.ts +++ b/src/server.ts @@ -58,7 +58,7 @@ export class SmppServer extends EventEmitter { private readonly server: NetServer; constructor(server: NetServer, log: SmppLog) { - super(); + super({ captureRejections: true }); this.log = log; this.server = server; } @@ -89,6 +89,18 @@ export class SmppServer extends EventEmitter { } } + /** The same guard for a listener that rejects rather than throws; captureRejections routes here. */ + override [EventEmitter.captureRejectionSymbol]( + error: Error, + ...args: [event: keyof ServerEvents, ...rest: unknown[]] + ): void { + const [event] = args; + + this.log.error('server - a listener rejected', { event, message: error.message }); + + if (event !== 'serverError') this.emit('serverError', error); + } + /** Stops listening and closes every live session. */ close(): Promise { return new Promise(resolve => { diff --git a/src/session.ts b/src/session.ts index 84ab472..93c492d 100644 --- a/src/session.ts +++ b/src/session.ts @@ -76,8 +76,20 @@ export class Session extends EventEmitter { } } + /** The same guard for a listener that rejects rather than throws; captureRejections routes here. */ + override [EventEmitter.captureRejectionSymbol]( + error: Error, + ...args: [event: keyof SessionEvents, ...rest: unknown[]] + ): void { + const [event] = args; + + this.log.error('session - a listener rejected', { event, message: error.message }); + + if (event !== 'sessionError') this.emit('sessionError', error); + } + constructor(options: SessionOptions) { - super(); + super({ captureRejections: true }); this.log = options.log ?? silentLog; this.options = options; diff --git a/test/session.test.ts b/test/session.test.ts index 16f9cc5..03a6224 100644 --- a/test/session.test.ts +++ b/test/session.test.ts @@ -1221,7 +1221,7 @@ describe('robustness', () => { }); }); -describe('application hooks that throw', () => { +describe('application hooks that throw or reject', () => { test('turns a throwing authenticate into a session error', async () => { const smpp = await startServer({ authenticate: () => { throw new Error('authenticate exploded'); }, @@ -1291,6 +1291,77 @@ describe('application hooks that throw', () => { await smpp.close(); }); + test('turns a rejecting async sms listener into a session error', async () => { + const smpp = await startServer(); + const failed = once(resolve => { + smpp.on('session', session => { + session.on('sessionError', resolve); + session.on('sms', async sms => { + await sms.sendResp(); + + throw new Error('listener rejected'); + }); + }); + }); + const { session } = await connect(smpp, { responseTimeout: 200 }); + + assert.ok(session); + + const sent = await session.sendSms({ + from: '46701113311', + message: 'rejects after answering', + to: '46709771337', + }); + const reported = await raceWithin(500, failed); + + assert.equal(sent.err, undefined); + assert.ok(reported instanceof Error, 'a rejecting sms listener should reach the session'); + assert.equal(reported.message, 'listener rejected'); + + session.close(); + await smpp.close(); + }); + + test('survives a sessionError listener that rejects as well', async () => { + const smpp = await startServer(); + + smpp.on('session', session => { + session.on('sessionError', () => Promise.reject(new Error('the reporter rejected too'))); + session.on('sms', () => Promise.reject(new Error('listener rejected'))); + }); + + const { session } = await connect(smpp, { responseTimeout: 200 }); + + assert.ok(session); + + const sent = await session.sendSms({ + from: '46701113311', + message: 'rejects in both listeners', + to: '46709771337', + }); + + assert.ok(sent.err instanceof Error); + + session.close(); + await smpp.close(); + }); + + test('turns a rejecting session listener into a server error', async () => { + const smpp = await startServer(); + const failed = once(resolve => { smpp.on('serverError', resolve); }); + + smpp.on('session', () => Promise.reject(new Error('session listener rejected'))); + + const { session } = await connect(smpp); + const reported = await raceWithin(500, failed); + + assert.ok(reported instanceof Error, 'a rejecting session listener should reach the server'); + assert.equal(reported.message, 'session listener rejected'); + + session?.close(); + await smpp.close(); + }); + test('closes even when an application close listener throws', async () => { const smpp = await startServer(); diff --git a/todo.md b/todo.md index 3d72fb8..1fa8fd9 100644 --- a/todo.md +++ b/todo.md @@ -5,7 +5,7 @@ rules there constrain every item below. ## Status -The rewrite is **feature complete and green**: 231 tests, lint and typecheck clean, verified on Node +The rewrite is **feature complete and green**: 234 tests, lint and typecheck clean, verified on Node 18, 20, 22 and 24. What is left is release work and a few things worth adding before or after 1.0.0. ```bash @@ -57,6 +57,7 @@ Rules the API follows: | Merged multipart DLRs, reconnect, reassembly bounds, per-send abort, the segment cap | `test/session-extras.test.ts` | | Every runnable README example | `test/readme.test.ts` | | Receipt-versus-message classification by `esm_class` | `test/dlr.test.ts`, `test/session.test.ts` | +| A listener that throws, or rejects, reaching `sessionError`/`serverError` rather than the process | `test/session.test.ts` | | Cross-checked against node-smpp both ways and over a live session | `test/interop.test.ts` | | CI on Node 18/20/22/24, Renovate, tag-triggered publish | `.github/workflows/` | @@ -110,15 +111,6 @@ session message is a change to every call site. ## Worth doing, not blocking -- [ ] **An `async` event listener that rejects escapes the guard.** `Session.emit()` wraps - `super.emit()` in try/catch, which catches a listener that throws synchronously but not one - that returns a rejected promise — that surfaces as an unhandled rejection and takes the - process down, which hard rule 1 says must not happen. Every README example uses - `session.on('sms', async sms => …)`, so the shape is the one applications will write. The - library's own calls inside such a listener never reject, so the examples themselves are safe. - Fixing it means dispatching `rawListeners()` by hand in `emit()` and routing a rejection to - `sessionError` — a change to the hottest path, so it needs a decision before 1.0.0. - - [ ] **In-flight sends across a reconnect.** They currently fail with "Session closed before a response arrived" and the caller retries. Re-queueing them automatically would be friendlier but risks duplicate delivery, so it needs a decision before it is built.