fix: call onerror callback before throwing transport errors#1704
fix: call onerror callback before throwing transport errors#1704ctonneslan wants to merge 2 commits intomodelcontextprotocol:mainfrom
Conversation
|
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
Several transport implementations threw errors without first calling the onerror callback, causing errors to be silently swallowed when callers didn't handle the thrown error. This adds proper onerror reporting to stdio, websocket, and streamable HTTP transports. Fixes modelcontextprotocol#1395
7f72486 to
9a8f2f8
Compare
travisbreaks
left a comment
There was a problem hiding this comment.
The core change is correct: calling onerror before throwing/rejecting ensures error handlers see the error before promise rejection propagates. This matches how Node.js streams work (emit 'error' before destroy).
Overlap note: PR #1705 by the same author includes all of these onerror additions plus the close() re-entrancy guard. If #1705 lands, this PR is redundant. Worth coordinating with the maintainers on which to merge first, or consolidating.
One concern on the event store wrapping:
try {
primingEventId = await this._eventStore.storeEvent(streamId, {} as JSONRPCMessage);
} catch (error) {
const err = error as Error;
this.onerror?.(err);
throw err;
}If this.onerror itself throws, the original error is lost. A safer pattern:
} catch (error) {
const err = error as Error;
try { this.onerror?.(err); } catch { /* handler error should not mask transport error */ }
throw err;
}This is defensive, but onerror is a user-provided callback so it could do anything.
Since onerror is a user-provided callback, it could throw. Without defensive wrapping, a throwing onerror handler would mask the original transport error (the reject/throw would never execute). Addresses review feedback from @travisbreaks.
ctonneslan
left a comment
There was a problem hiding this comment.
Good catch on the defensive wrapping — pushed a fix. All 10 onerror call sites across the 4 transport files are now wrapped in try/catch so a throwing user callback can't mask the original transport error.
Re: overlap with #1705 — this PR is actually complementary, not redundant. #1705 fixes close() re-entrancy, this one fixes send() error handling. They touch different code paths. Updated #1705's description to note the relationship.
Summary
onerrorcallback, causing errors to be silently swallowedonerrorreporting tosend()methods in:StdioClientTransport(client stdio)StdioServerTransport(server stdio)WebSocketClientTransport(client websocket)StreamableHTTPServerTransport(server streamable HTTP)Fixes #1395
Test plan
send()are reported viaonerrorbefore propagating