Skip to content

Commit 745ed69

Browse files
committed
Contain notification-handler exceptions in the dispatcher
A raising notification handler ran as a bare task in the dispatcher's task group, so its exception cancelled the read loop and every in-flight request. Wrap spawned handlers in the same containment boundary progress callbacks already have: log the failure and keep the connection serving.
1 parent ca9e87f commit 745ed69

2 files changed

Lines changed: 64 additions & 2 deletions

File tree

src/mcp/shared/jsonrpc_dispatcher.py

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@
3434

3535
from mcp.shared._otel import inject_trace_context, otel_span
3636
from mcp.shared._stream_protocols import ReadStream, WriteStream
37-
from mcp.shared.dispatcher import CallOptions, Dispatcher, OnNotify, OnRequest, ProgressFnT
37+
from mcp.shared.dispatcher import CallOptions, DispatchContext, Dispatcher, OnNotify, OnRequest, ProgressFnT
3838
from mcp.shared.exceptions import MCPError, NoBackChannelError
3939
from mcp.shared.message import (
4040
ClientMessageMetadata,
@@ -190,6 +190,18 @@ async def _wrapped(progress: float, total: float | None, message: str | None) ->
190190
return _wrapped
191191

192192

193+
def _contained_notify(fn: OnNotify) -> OnNotify:
194+
"""Wrap a notification handler so it can't crash the dispatcher (same boundary as `_shielded_progress`)."""
195+
196+
async def _wrapped(dctx: DispatchContext[TransportContext], method: str, params: Mapping[str, Any] | None) -> None:
197+
try:
198+
await fn(dctx, method, params)
199+
except Exception:
200+
logger.exception("notification handler for %r raised", method)
201+
202+
return _wrapped
203+
204+
193205
def _outbound_metadata(related_request_id: RequestId | None, opts: CallOptions | None) -> MessageMetadata:
194206
"""Choose the `SessionMessage.metadata` for an outgoing request/notification.
195207
@@ -619,7 +631,7 @@ def _dispatch_notification(
619631
dctx = _JSONRPCDispatchContext(
620632
transport=transport_ctx, _dispatcher=self, _request_id=None, message_metadata=metadata
621633
)
622-
self._spawn(on_notify, dctx, msg.method, msg.params, sender_ctx=sender_ctx)
634+
self._spawn(_contained_notify(on_notify), dctx, msg.method, msg.params, sender_ctx=sender_ctx)
623635

624636
def _resolve_pending(self, request_id: RequestId | None, outcome: dict[str, Any] | ErrorData) -> None:
625637
pending = self._pending.get(_coerce_id(request_id)) if request_id is not None else None

tests/shared/test_jsonrpc_dispatcher.py

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -610,6 +610,56 @@ async def caller() -> None:
610610
assert scopes[0].cancelled_caught
611611

612612

613+
@pytest.mark.anyio
614+
async def test_notification_handler_exception_is_contained(caplog: pytest.LogCaptureFixture):
615+
"""A raising notification handler costs only that notification, never the connection.
616+
617+
The handler runs as a bare task in the dispatcher's task group; without containment its
618+
exception would cancel the read loop and every in-flight request. The TypeScript, C#, and
619+
Go engines all contain notification-handler failures the same way.
620+
"""
621+
622+
async def server_on_notify(ctx: DCtx, method: str, params: Mapping[str, Any] | None) -> None:
623+
raise RuntimeError("notify boom")
624+
625+
async with running_pair(jsonrpc_pair, server_on_notify=server_on_notify) as (client, *_):
626+
with anyio.fail_after(5):
627+
await client.notify("boom", None)
628+
# The connection survived: a full round-trip still works.
629+
result = await client.send_raw_request("ping", None)
630+
assert result == {"echoed": "ping", "params": {}}
631+
assert "notification handler for 'boom' raised" in caplog.text
632+
633+
634+
@pytest.mark.anyio
635+
async def test_spawned_notification_handlers_run_concurrently():
636+
"""Notification handlers are spawned, not serialized: a parked one does not block the next.
637+
638+
The first handler waits for the second to have started - serialized dispatch would deadlock
639+
here. This matches the TypeScript and C# engines (fire-and-forget); handlers needing
640+
mutual ordering must coordinate themselves.
641+
"""
642+
second_started = anyio.Event()
643+
completed: list[str] = []
644+
done = anyio.Event()
645+
646+
async def server_on_notify(ctx: DCtx, method: str, params: Mapping[str, Any] | None) -> None:
647+
if method == "first":
648+
await second_started.wait()
649+
else:
650+
second_started.set()
651+
completed.append(method)
652+
if len(completed) == 2:
653+
done.set()
654+
655+
async with running_pair(jsonrpc_pair, server_on_notify=server_on_notify) as (client, *_):
656+
with anyio.fail_after(5):
657+
await client.notify("first", None)
658+
await client.notify("second", None)
659+
await done.wait()
660+
assert completed == ["second", "first"]
661+
662+
613663
@pytest.mark.anyio
614664
async def test_ctx_message_metadata_carries_inbound_request_metadata():
615665
"""Transport-attached metadata (HTTP request, SSE close hooks) is readable off the dispatch context."""

0 commit comments

Comments
 (0)