- Notifications
You must be signed in to change notification settings - Fork2.1k
Fix streamable http sampling#693
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Thanks for the quick response@ihrpr! Confirm this fixes both my test case and the original reported bug in the FastMCP repo 🎉 |
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.
Looks allgood to me - one non-blocking question around breaking/awaiting message handling in the client
@@ -309,6 +310,8 @@ async def _handle_sse_response( | |||
else None | |||
), | |||
) | |||
if is_complete: | |||
break |
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 break here?
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.
_handle_sse_event
returns true if there is a response/error. this means we are done and need to stop the stream
c6fb822
intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Thank you@jlowin for raising#680 🙏
The StreamableHTTP client was experiencing a deadlock when the server initiated sampling callbacks during request
processing. The issue occurred due to synchronous request handling in the client's post_writer function.
The Deadlock Scenario:
The circular dependency prevented the sampling response from ever reaching the server, causing the system to hang
indefinitely.
The Solution
Concurrent request handling in the StreamableHTTP client:
tasks using tg.start_soon(), while responses continue to be handled synchronously (notification need to be handled concurrently as we will have other issues like initialisation notifications )
The fix ensures that request handling doesn't block the message processing pipeline, allowing the client to:
Follow ups: