-
Notifications
You must be signed in to change notification settings - Fork 160
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add request cancellation and cleanup #167
base: main
Are you sure you want to change the base?
Conversation
0c6ec6d
to
d50255e
Compare
0d7be0a
to
0a93799
Compare
async def _cleanup_loop(self) -> None: | ||
"""Periodically clean up completed and cancelled requests.""" | ||
while True: | ||
with anyio.move_on_after(self._cleanup_interval): | ||
# Clean up completed requests | ||
self._in_flight = { | ||
req_id: responder | ||
for req_id, responder in self._in_flight.items() | ||
if responder.in_flight | ||
} | ||
await anyio.sleep(self._cleanup_interval) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why isn't this done on completion or cancellation instead? The idea of using a timer for this seems weird to me, and leaves a window (the length of the interval) of possibly unbounded memory growth.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Clean up completed requests | ||
self._in_flight = { | ||
req_id: responder | ||
for req_id, responder in self._in_flight.items() | ||
if responder.in_flight | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OTOH, this seems excessive to do on every new incoming message. Why can't we tie it to completion and cancellation?
This doesn't seem to solve #88 for me. I can reproduce triggering a call tool from claude with a call tool handle that simply waits for 200 s and returns a valid message. Claude receives the response but the server crashes @server.call_tool()
async def handle_call_tool(
name: str, arguments: dict | None,
) -> list[types.TextContent | types.ImageContent | types.EmbeddedResource]:
await asyncio.sleep(200)
response = {"msg": "success"}
return [types.TextContent(type="text", text=f"{response}")] |
Summary
Test plan
Depends on #166