-
Notifications
You must be signed in to change notification settings - Fork 186
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
End to end logging #202
base: main
Are you sure you want to change the base?
End to end logging #202
Conversation
… after receiving it. This seems to create deadlock.
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.
In gneeral I am not too keen on end to end tests that involve multiple files but are disguised as unit tests. We should see if we can make this test work within one file by using memory streams, etc.
# await self._incoming_message_stream_writer.send( | ||
# notification | ||
# ) |
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.
That seems to be a debugging left over?
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.
These are the lines that are present in shared/session that I believe cause the deadlock. If you uncomment the lines and run the end-to-end test, it will not complete (it will hang). If you comment the lines out, the test runs to completion.
|
||
from contextlib import AsyncExitStack | ||
from typing import Any | ||
import asyncio |
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.
We generally don't use asyncio but anyio.
Agreed, a more directed unit test might be better going forward after the issue is understood. The scenario in this end-to-end test illustrates the deadlock with enough context to reproduce the issue described in #201 when run. |
That makes sense. Can we move this over to DRAFT status until it's ready to be properly reviewed? |
This PR should probably be viewed as a bug-recreation rather than code that should be merged. The end-to-end test does a good job at illustrating the deadlock, I believe. |
This addresses Issue #201 and provides a test and a fix.
Motivation and Context
An MCP server may include logging messages (notifications/message) by using the ctx.{info,debug}() call.
Currently it seems that Claude Desktop and Inspector operate properly when an MCP server sends these
notifications, even if those clients do not do anything with them.
In the process of writing a Pure-Python client using the example given in simple-chatbot as a starting point,
if logging notifications are sent, they cause the client to hang.
There is an included fix in the shared/session.py file. I believe the acknowledgement of the received notification is causing a deadlock. Try running the test without the fix to observe server hanging.
The goal is to simply log the received notifications in the Client, and you can see that in the NotificationLoggingClientSession
How Has This Been Tested?
The included test illustrates the pattern.
Breaking Changes
possibly
Types of changes
Checklist
Additional context