Skip to content

Commit bf42fa1

Browse files
committed
Fix UnboundLocalError in send_request when response not assigned
Fixes an intermittent UnboundLocalError that could occur in send_request() when anyio's fail_after context manager incorrectly suppresses exceptions due to a race condition (anyio#589). Changes: - Initialize response_or_error to None before the try block - Add _process_response static method with defensive None check - Handle EndOfStream and ClosedResourceError exceptions explicitly - Use try/except/else structure for cleaner control flow - Add unit tests for _process_response Closes #1717
1 parent 8e02fc1 commit bf42fa1

File tree

2 files changed

+99
-5
lines changed

2 files changed

+99
-5
lines changed

src/mcp/shared/session.py

Lines changed: 48 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
from mcp.shared.response_router import ResponseRouter
1717
from mcp.types import (
1818
CONNECTION_CLOSED,
19+
INTERNAL_ERROR,
1920
INVALID_PARAMS,
2021
CancelledNotification,
2122
ClientNotification,
@@ -237,6 +238,34 @@ async def __aexit__(
237238
self._task_group.cancel_scope.cancel()
238239
return await self._task_group.__aexit__(exc_type, exc_val, exc_tb)
239240

241+
@staticmethod
242+
def _process_response(
243+
response_or_error: JSONRPCResponse | JSONRPCError | None,
244+
result_type: type[ReceiveResultT],
245+
) -> ReceiveResultT:
246+
"""
247+
Process a JSON-RPC response, validating and returning the result.
248+
249+
Raises McpError if the response is an error or if response_or_error is None.
250+
The None check is a defensive guard against anyio race conditions - see #1717.
251+
"""
252+
if response_or_error is None:
253+
# Defensive check for anyio fail_after race condition (#1717).
254+
# If anyio's CancelScope incorrectly suppresses an exception,
255+
# the response variable may never be assigned. See:
256+
# https://github.com/agronholm/anyio/issues/589
257+
raise McpError(
258+
ErrorData(
259+
code=INTERNAL_ERROR,
260+
message="Internal error: no response received",
261+
)
262+
)
263+
264+
if isinstance(response_or_error, JSONRPCError):
265+
raise McpError(response_or_error.error)
266+
267+
return result_type.model_validate(response_or_error.result)
268+
240269
async def send_request(
241270
self,
242271
request: SendRequestT,
@@ -287,6 +316,10 @@ async def send_request(
287316
elif self._session_read_timeout_seconds is not None: # pragma: no cover
288317
timeout = self._session_read_timeout_seconds.total_seconds()
289318

319+
# Initialize to None as a defensive guard against anyio race conditions
320+
# where fail_after may incorrectly suppress exceptions (#1717)
321+
response_or_error: JSONRPCResponse | JSONRPCError | None = None
322+
290323
try:
291324
with anyio.fail_after(timeout):
292325
response_or_error = await response_stream_reader.receive()
@@ -301,12 +334,22 @@ async def send_request(
301334
),
302335
)
303336
)
304-
305-
if isinstance(response_or_error, JSONRPCError):
306-
raise McpError(response_or_error.error)
337+
except anyio.EndOfStream:
338+
raise McpError(
339+
ErrorData(
340+
code=CONNECTION_CLOSED,
341+
message="Connection closed: stream ended unexpectedly",
342+
)
343+
)
344+
except anyio.ClosedResourceError:
345+
raise McpError(
346+
ErrorData(
347+
code=CONNECTION_CLOSED,
348+
message="Connection closed",
349+
)
350+
)
307351
else:
308-
return result_type.model_validate(response_or_error.result)
309-
352+
return self._process_response(response_or_error, result_type)
310353
finally:
311354
self._response_streams.pop(request_id, None)
312355
self._progress_callbacks.pop(request_id, None)

tests/shared/test_session.py

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,17 @@
99
from mcp.server.lowlevel.server import Server
1010
from mcp.shared.exceptions import McpError
1111
from mcp.shared.memory import create_client_server_memory_streams, create_connected_server_and_client_session
12+
from mcp.shared.session import BaseSession
1213
from mcp.types import (
14+
INTERNAL_ERROR,
1315
CancelledNotification,
1416
CancelledNotificationParams,
1517
ClientNotification,
1618
ClientRequest,
1719
EmptyResult,
20+
ErrorData,
21+
JSONRPCError,
22+
JSONRPCResponse,
1823
TextContent,
1924
)
2025

@@ -168,3 +173,49 @@ async def mock_server():
168173
await ev_closed.wait()
169174
with anyio.fail_after(1):
170175
await ev_response.wait()
176+
177+
178+
class TestProcessResponse:
179+
"""Tests for BaseSession._process_response static method."""
180+
181+
def test_process_response_with_valid_response(self):
182+
"""Test that a valid JSONRPCResponse is processed correctly."""
183+
response = JSONRPCResponse(
184+
jsonrpc="2.0",
185+
id=1,
186+
result={},
187+
)
188+
189+
result = BaseSession._process_response(response, EmptyResult)
190+
191+
assert isinstance(result, EmptyResult)
192+
193+
def test_process_response_with_error(self):
194+
"""Test that a JSONRPCError raises McpError."""
195+
error = JSONRPCError(
196+
jsonrpc="2.0",
197+
id=1,
198+
error=ErrorData(code=-32600, message="Invalid request"),
199+
)
200+
201+
with pytest.raises(McpError) as exc_info:
202+
BaseSession._process_response(error, EmptyResult)
203+
204+
assert exc_info.value.error.code == -32600
205+
assert exc_info.value.error.message == "Invalid request"
206+
207+
def test_process_response_with_none(self):
208+
"""
209+
Test defensive check for anyio fail_after race condition (#1717).
210+
211+
If anyio's CancelScope incorrectly suppresses an exception during
212+
receive(), the response variable may never be assigned. This test
213+
verifies we handle this gracefully instead of raising UnboundLocalError.
214+
215+
See: https://github.com/agronholm/anyio/issues/589
216+
"""
217+
with pytest.raises(McpError) as exc_info:
218+
BaseSession._process_response(None, EmptyResult)
219+
220+
assert exc_info.value.error.code == INTERNAL_ERROR
221+
assert "no response received" in exc_info.value.error.message

0 commit comments

Comments
 (0)