From 344249b080bcbd74f3b75b61b67b837bf5858dcf Mon Sep 17 00:00:00 2001 From: Lilleman auf Larv Date: Thu, 27 Aug 2026 23:07:48 +0200 Subject: [PATCH] Accept an async listener in the types and normalise any rejection reason --- AGENTS.md | 16 +++++++++++++--- eslint.config.js | 7 ------- src/error-from.ts | 10 ++++++++++ src/server.ts | 20 ++++++++++++++++---- src/session.ts | 18 +++++++++++++++--- test/error-from.test.ts | 17 +++++++++++++++++ todo.md | 12 ++---------- 7 files changed, 73 insertions(+), 27 deletions(-) create mode 100644 src/error-from.ts create mode 100644 test/error-from.test.ts diff --git a/AGENTS.md b/AGENTS.md index d9936b1..4019f82 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -42,6 +42,7 @@ src/ sms.ts The live handle emitted as the 'sms' event (sendResp/sendDlr) dlr.ts Delivery receipts: text and TLV parsing, receipt status codes dlr-merger.ts DlrMerger: per-segment receipts counted into one MessageDlr + error-from.ts errorFrom(): whatever was thrown or rejected, as an Error expiring-groups.ts ExpiringGroups: the capped, expiring store both of those share incoming-requests.ts Every request the peer sends: messages, receipts, links, unknown commands link-timers.ts LinkTimers: the enquire_link heartbeat and the idle timeout @@ -218,9 +219,18 @@ exactly 140. `[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. A rejection reason is `unknown`, so the handler normalises it the way the synchronous - guard does. `Session.on()` still types its listeners as void-returning, because widening that needs - the same cast — which is why `test/*.test.ts` turns off `no-misused-promises` on arguments. + forbids. A rejection reason is `unknown` and `String()` throws on a null-prototype object, so both + handlers normalise through `errorFrom()` rather than inline — a route out of the handler would land + on a bare `process.nextTick` with nothing to catch it. + +- **Both emitters re-declare their listener methods to accept a promise.** Maintainer's call, + 2026-08-27: `EventEmitter` types every listener as void-returning, so the + `session.on('sms', async sms => …)` the README documents reads as a misused promise in any strict + consumer. `declare on: …` and its six siblings re-type the inherited methods to return `unknown`, + which emits nothing, needs no cast and leaves the runtime method on the prototype. Overriding them + as real methods instead cannot work: the `super.on()` call needs a cast to satisfy the conditional + `Listener` type. `unknown` rather than `void | Promise` because a listener may return + anything — `session.on('close', () => set.delete(session))` returns a boolean. - **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 diff --git a/eslint.config.js b/eslint.config.js index aa07c67..755c9c0 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -48,13 +48,6 @@ export default tseslint.config( files: ['src/defs/encodings.ts'], rules: { 'no-control-regex': 'off' }, }, - { - // An async event listener is a documented, supported shape; EventEmitter types listeners void. - files: ['test/*.test.ts'], - rules: { - '@typescript-eslint/no-misused-promises': ['error', { checksVoidReturn: { arguments: false } }], - }, - }, { files: ['eslint.config.js'], extends: [tseslint.configs.disableTypeChecked], diff --git a/src/error-from.ts b/src/error-from.ts new file mode 100644 index 0000000..a091479 --- /dev/null +++ b/src/error-from.ts @@ -0,0 +1,10 @@ +/** Whatever was thrown or rejected, as an Error. `String()` throws on some values; this cannot. */ +export function errorFrom(reason: unknown): Error { + if (reason instanceof Error) return reason; + + try { + return new Error(String(reason)); + } catch { + return new Error('A thrown value that cannot be converted to a string'); + } +} diff --git a/src/server.ts b/src/server.ts index aef8b55..bad202e 100644 --- a/src/server.ts +++ b/src/server.ts @@ -9,6 +9,7 @@ import { bindTypeFromCommand, checkSessionOptions, undeclaredInterfaceVersion } import { createServer as createNetServer } from 'node:net'; import { createServer as createTlsServer } from 'node:tls'; import { defaultInterfaceVersion } from './defs/constants.ts'; +import { errorFrom } from './error-from.ts'; import { paramText } from './defs/types.ts'; import { silentLog } from './log.ts'; @@ -50,8 +51,19 @@ const defaults = { systemId: defaultSystemId, }; +/** A listener may return a promise: an `async` one that rejects is routed like one that throws. */ +type ServerListener = (...args: ServerEvents[K]) => unknown; + /** A listening SMPP server. Sessions arrive as `session` events; `close()` stops listening. */ export class SmppServer extends EventEmitter { + declare addListener: (event: K, listener: ServerListener) => this; + declare off: (event: K, listener: ServerListener) => this; + declare on: (event: K, listener: ServerListener) => this; + declare once: (event: K, listener: ServerListener) => this; + declare prependListener: (event: K, listener: ServerListener) => this; + declare prependOnceListener: (event: K, listener: ServerListener) => this; + declare removeListener: (event: K, listener: ServerListener) => this; + readonly sessions = new Set(); private readonly log: SmppLog; @@ -78,7 +90,7 @@ export class SmppServer extends EventEmitter { try { return super.emit(event, ...args); } catch (thrown: unknown) { - const err = thrown instanceof Error ? thrown : new Error(String(thrown)); + const err = errorFrom(thrown); this.log.error('server - a listener threw', { event, message: err.message }); @@ -95,7 +107,7 @@ export class SmppServer extends EventEmitter { ...args: [event: keyof ServerEvents, ...rest: unknown[]] ): void { const [event] = args; - const error = reason instanceof Error ? reason : new Error(String(reason)); + const error = errorFrom(reason); this.log.error('server - a listener rejected', { event, message: error.message }); @@ -109,7 +121,7 @@ export class SmppServer extends EventEmitter { try { session.close(); } catch (thrown: unknown) { - this.emit('serverError', thrown instanceof Error ? thrown : new Error(String(thrown))); + this.emit('serverError', errorFrom(thrown)); } } @@ -332,7 +344,7 @@ export function server(options: ServerOptions = {}): Promise = (...args: SessionEvents[K]) => unknown; + export class Session extends EventEmitter { + declare addListener: (event: K, listener: SessionListener) => this; + declare off: (event: K, listener: SessionListener) => this; + declare on: (event: K, listener: SessionListener) => this; + declare once: (event: K, listener: SessionListener) => this; + declare prependListener: (event: K, listener: SessionListener) => this; + declare prependOnceListener: (event: K, listener: SessionListener) => this; + declare removeListener: (event: K, listener: SessionListener) => this; + /** Replaced on reconnect, so hold the session rather than this. */ sock: Socket; readonly log: SmppLog; @@ -65,7 +77,7 @@ export class Session extends EventEmitter { try { return super.emit(event, ...args); } catch (thrown: unknown) { - const err = thrown instanceof Error ? thrown : new Error(String(thrown)); + const err = errorFrom(thrown); this.log.error('session - a listener threw', { event, message: err.message }); @@ -82,7 +94,7 @@ export class Session extends EventEmitter { ...args: [event: keyof SessionEvents, ...rest: unknown[]] ): void { const [event] = args; - const error = reason instanceof Error ? reason : new Error(String(reason)); + const error = errorFrom(reason); this.log.error('session - a listener rejected', { event, message: error.message }); @@ -379,7 +391,7 @@ export class Session extends EventEmitter { this.emit('incomingPduObj', pduObj); // Every application hook and listener reached from an incoming PDU funnels through here. void this.incoming.handle(pduObj).catch((thrown: unknown) => { - const err = thrown instanceof Error ? thrown : new Error(String(thrown)); + const err = errorFrom(thrown); this.log.error('session - a handler threw', { message: err.message }); this.emit('sessionError', err); diff --git a/test/error-from.test.ts b/test/error-from.test.ts new file mode 100644 index 0000000..6a98889 --- /dev/null +++ b/test/error-from.test.ts @@ -0,0 +1,17 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; +import { errorFrom } from '../src/error-from.ts'; + +test('carries an Error through and describes anything else, including what String() refuses', () => { + const original = new Error('the original'); + const undescribable: unknown = Object.create(null); + + assert.equal(errorFrom(original), original); + assert.equal(errorFrom('a string').message, 'a string'); + assert.equal(errorFrom(null).message, 'null'); + assert.equal(errorFrom(undefined).message, 'undefined'); + assert.equal( + errorFrom(undescribable).message, + 'A thrown value that cannot be converted to a string', + ); +}); diff --git a/todo.md b/todo.md index 424730f..66bebd9 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**: 234 tests, lint and typecheck clean, verified on Node +The rewrite is **feature complete and green**: 235 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,7 +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` | +| A listener that throws, or rejects, reaching `sessionError`/`serverError` rather than the process | `test/session.test.ts`, `test/error-from.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/` | @@ -111,14 +111,6 @@ session message is a change to every call site. ## Worth doing, not blocking -- [ ] **Should `on()` accept a promise-returning listener in its types?** A listener that rejects is - routed to `sessionError` now, but `EventEmitter` types every listener as void-returning, so the - `session.on('sms', async sms => …)` the README documents trips `no-misused-promises` in a - consumer's project exactly as it does in this repository's own tests. Widening the parameter - needs a cast, which hard rule 4 forbids; a declaration overload over an implementation - signature Node's types accept might not. Raised by review, 2026-08-27; worth deciding while - the surface is still movable. - - [ ] **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.