Skip to content

Commit d2f7de8

Browse files
Add tests for resource leak in streamable_http SSE handlers
Fixes #1450 - HTTP responses don't get closed properly when SSE streaming fails with exceptions in _handle_sse_response and _handle_resumption_request. The issue: when the async for loop throws an exception, response.aclose() never gets called because it's only in the success path. Added reproduction script and pytest tests to demonstrate the problem. The fix requires adding finally blocks to ensure response.aclose() always gets called.
1 parent dcc68ce commit d2f7de8

File tree

2 files changed

+445
-0
lines changed

2 files changed

+445
-0
lines changed

resource_leak_reproduction.py

Lines changed: 230 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,230 @@
1+
#!/usr/bin/env python3
2+
"""
3+
Reproduction script for the resource leak I found in streamable_http.py
4+
5+
I noticed that when SSE streaming fails, the HTTP response doesn't get closed properly.
6+
This happens in both _handle_sse_response and _handle_resumption_request methods.
7+
8+
The problem: if the async for loop throws an exception (like malformed JSON or network issues),
9+
the response.aclose() call never happens because it's only in the success path.
10+
11+
Files affected:
12+
- src/mcp/client/streamable_http.py (lines 336 and 251)
13+
14+
This can cause connection pool exhaustion over time in production.
15+
"""
16+
17+
import asyncio
18+
import sys
19+
from pathlib import Path
20+
21+
# Add the mcp module to the path
22+
sys.path.insert(0, str(Path(__file__).parent / "src"))
23+
24+
from mcp.client.streamable_http import StreamableHTTPTransport
25+
26+
27+
class MockResponse:
28+
"""Simple mock to track if aclose() gets called"""
29+
30+
def __init__(self):
31+
self.closed = False
32+
self.close_count = 0
33+
34+
async def aclose(self):
35+
self.closed = True
36+
self.close_count += 1
37+
print(f"Response closed (called {self.close_count} times)")
38+
39+
40+
class MockEventSource:
41+
"""Mock that throws an exception to simulate broken SSE"""
42+
43+
def __init__(self, response):
44+
self.response = response
45+
46+
def __aiter__(self):
47+
return self
48+
49+
async def __anext__(self):
50+
# Simulate what happens when SSE parsing fails
51+
raise Exception("SSE parsing failed - connection broken")
52+
53+
54+
class MockTransport(StreamableHTTPTransport):
55+
"""Mock that shows the same bug as the real code"""
56+
57+
def __init__(self):
58+
super().__init__("http://test")
59+
self.mock_response = MockResponse()
60+
61+
async def _handle_sse_response(self, response, ctx, is_initialization=False):
62+
"""
63+
This mimics the actual bug in the real code.
64+
65+
The problem: when the async for loop throws an exception,
66+
response.aclose() never gets called because it's only in the success path.
67+
"""
68+
try:
69+
event_source = MockEventSource(response)
70+
async for sse in event_source:
71+
# This never runs because the exception happens first
72+
is_complete = False
73+
if is_complete:
74+
await response.aclose() # This is line 336 in the real code
75+
break
76+
except Exception as e:
77+
print(f"Exception caught: {e}")
78+
# Here's the bug - response.aclose() is never called!
79+
raise
80+
81+
async def _handle_resumption_request(self, ctx):
82+
"""
83+
Same issue here - the aconnect_sse context manager should handle cleanup,
84+
but if exceptions happen during SSE iteration, the response might not get closed.
85+
"""
86+
try:
87+
# Mock the aconnect_sse context manager
88+
class MockEventSourceWithResponse:
89+
def __init__(self, response):
90+
self.response = response
91+
92+
async def __aenter__(self):
93+
return self
94+
95+
async def __aexit__(self, exc_type, exc_val, exc_tb):
96+
# Context manager exits but response might not be closed
97+
pass
98+
99+
def __aiter__(self):
100+
return self
101+
102+
async def __anext__(self):
103+
raise Exception("Resumption SSE parsing failed")
104+
105+
async with MockEventSourceWithResponse(self.mock_response) as event_source:
106+
async for sse in event_source:
107+
# This never runs because the exception happens first
108+
is_complete = False
109+
if is_complete:
110+
await event_source.response.aclose() # This is line 251 in the real code
111+
break
112+
except Exception as e:
113+
print(f"Exception caught: {e}")
114+
# Same bug here - response.aclose() is never called!
115+
raise
116+
117+
118+
async def test_resource_leak():
119+
"""Test the resource leak I found"""
120+
print("Testing resource leak in streamable_http.py")
121+
print("=" * 50)
122+
123+
transport = MockTransport()
124+
125+
# Create mock context
126+
class MockContext:
127+
def __init__(self):
128+
self.read_stream_writer = None
129+
self.metadata = None
130+
131+
ctx = MockContext()
132+
133+
print("\nTesting _handle_sse_response method:")
134+
print("-" * 35)
135+
136+
try:
137+
await transport._handle_sse_response(transport.mock_response, ctx)
138+
except Exception as e:
139+
print(f"Caught expected exception: {e}")
140+
141+
# Check if response was closed
142+
if transport.mock_response.closed:
143+
print("No resource leak - response was closed properly")
144+
return True
145+
else:
146+
print("RESOURCE LEAK DETECTED!")
147+
print(f" Response closed: {transport.mock_response.closed}")
148+
print(f" Close count: {transport.mock_response.close_count}")
149+
print(" Expected: response.aclose() to be called in finally block")
150+
return False
151+
152+
153+
async def test_resumption_resource_leak():
154+
"""Test the resource leak in _handle_resumption_request"""
155+
print("\nTesting _handle_resumption_request method:")
156+
print("-" * 40)
157+
158+
transport = MockTransport()
159+
160+
# Create mock context with resumption token
161+
class MockResumptionContext:
162+
def __init__(self):
163+
self.read_stream_writer = None
164+
self.metadata = type("obj", (object,), {"resumption_token": "test-token"})()
165+
self.session_message = type(
166+
"obj",
167+
(object,),
168+
{"message": type("obj", (object,), {"root": type("obj", (object,), {"id": "test-id"})()})()},
169+
)()
170+
171+
ctx_resumption = MockResumptionContext()
172+
173+
try:
174+
await transport._handle_resumption_request(ctx_resumption)
175+
except Exception as e:
176+
print(f"Caught expected exception: {e}")
177+
178+
# Check if response was closed
179+
if transport.mock_response.closed:
180+
print("No resource leak - response was closed properly")
181+
return True
182+
else:
183+
print("RESOURCE LEAK DETECTED!")
184+
print(f" Response closed: {transport.mock_response.closed}")
185+
print(f" Close count: {transport.mock_response.close_count}")
186+
print(" Expected: response.aclose() to be called in finally block")
187+
return False
188+
189+
190+
async def main():
191+
"""Run the tests to show the resource leak"""
192+
print("Resource Leak Test")
193+
print("This shows the issue I found where HTTP responses don't get closed")
194+
print("when SSE streaming fails in the MCP Python SDK.")
195+
print()
196+
197+
# Test both methods
198+
sse_leak = await test_resource_leak()
199+
resumption_leak = await test_resumption_resource_leak()
200+
201+
print("\n" + "=" * 50)
202+
print("SUMMARY:")
203+
print("=" * 50)
204+
205+
if sse_leak and resumption_leak:
206+
print("All tests passed - no resource leaks detected")
207+
return 0
208+
else:
209+
print("Resource leaks confirmed in the following methods:")
210+
if not sse_leak:
211+
print(" - _handle_sse_response (line 336)")
212+
if not resumption_leak:
213+
print(" - _handle_resumption_request (line 251)")
214+
print()
215+
print("FIX NEEDED:")
216+
print(" Add finally blocks to ensure response.aclose() is always called:")
217+
print(" ```python")
218+
print(" try:")
219+
print(" # ... existing code ...")
220+
print(" except Exception as e:")
221+
print(" # ... existing exception handling ...")
222+
print(" finally:")
223+
print(" await response.aclose()")
224+
print(" ```")
225+
return 1
226+
227+
228+
if __name__ == "__main__":
229+
exit_code = asyncio.run(main())
230+
sys.exit(exit_code)

0 commit comments

Comments
 (0)