modelcontextprotocol / modelcontextprotocol/typescript-sdk

Promise/async handling causes unhandled rejections

Open
#392 4 comments 2 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug fix proposed P2 ready for work
Dominant language
TypeScript
Stars
13.4k
Forks
2.2k
Avg merge
3d 15h
Merged PRs (30d)
4

Description

Describe the bug

If an error occurs while sending an error response in a request handler() it remains unhandled if there's no onerror handler on the protocol (which is the default)

This means that nothing is returned to the client and the promise rejection goes unhandled. On Cloudflare Workers, this will result in an error like this: A hanging Promise was canceled. This happens when the worker runtime is waiting for a Promise from JavaScript to resolve, but has detected that the Promise cannot possibly ever resolve because all code and events related to the Promise's I/O context have already finished., and on Node.js, I suspect this would result in an unhandledRejection event though I haven't confirmed this – it may just be that the request stays open and eventually times out.

The problem is it's very hard to know exactly what's happened (or even that adding an onerror handler may help)

Expected behavior

Currently the protocol fires off Promises without awaiting them. Messages are processed in a loop without waiting, which means they all get processed in parallel, with no limits or waiting for execution. And the _onrequest handler creates a Promise without awaiting or returning it.

This kind of event handling becomes difficult to manage unless all event handlers ultimately resolve (including error handling). In the case of unhandled rejections, it's often not clear where the rejection happened, if it's even logged / managed at all – the error could just disappear into the ether if there's no error handler.

It also means it's very easy for errors to be handled multiple times, and if each handler writes a message on the transport, you then end up with Cannot write headers after they are sent to the client.

Ideally, install default error handlers that can handle an error occurring anywhere and won't leave unhandled promises/rejections. Having a default handler that at least does console.error would ensure that users should see the error, even if it's not handled gracefully.

Longer term I'd love to see this refactored to use async/await instead of events – it should be much easier to reason about, much easier to handle concurrency and flow control (eg message processing), and much easier to ensure errors or responses aren't sent over the wire multiple times.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by reading the request handler in src/shared/protocol.ts around lines 374-411 and the message-processing loop in src/server/streamableHttp.ts around lines 429-431. Trace how rejected Promises and errors flow when no onerror handler is configured. Done means failures are surfaced or handled without unhandled rejections, missing client responses, or duplicate transport writes.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
api, backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.