Title: Fix #201: Move incoming message stream from BaseSession to ServerSession by dsp-ant · Pull Request #325 · modelcontextprotocol/python-sdk · GitHub
Open Graph Title: Fix #201: Move incoming message stream from BaseSession to ServerSession by dsp-ant · Pull Request #325 · modelcontextprotocol/python-sdk
X Title: Fix #201: Move incoming message stream from BaseSession to ServerSession by dsp-ant · Pull Request #325 · modelcontextprotocol/python-sdk
Description: This PR fixes a nasty but in the way we handle incoming messages, which currently leads to hanging connections when the clients sends notifications. The observed issue is as follows: A user creates a ClientSession and connects to a server. Server sends messages such as logging events. Upon then next request from the client, the client await hangs. When I looked into this, I suspected that this must be some channel issues. We use in memory channels to handle messages and it has historically caused issues. In particular, we use channels with a max_lenght of 0, effectively making them non buffered. This is one, if not the only part of the codebase that can block as observed. So I looked into the two parts where we use channels, once for read and write stream from the sse/stdio implementation to the ClientSession and then within the shared BaseSession class to handle messages separately in the server and client session. I first changed in client/stdio.py the lines: read_stream_writer, read_stream = anyio.create_memory_object_stream(0) write_stream, write_stream_reader = anyio.create_memory_object_stream(0) to read_stream_writer, read_stream = anyio.create_memory_object_stream(math.inf) write_stream, write_stream_reader = anyio.create_memory_object_stream(math.inf) and tested again. This fixed the bug! However clearly we are now in bounded memory stream territory which we cannot do without growing buffers in memory linearly. I tested da bit more and boiled it down to read_stream_writer, read_stream = anyio.create_memory_object_stream(math.inf) fixing the issue. So we know something is funky in the read stream. I looked into @sheffler's issue #201 and PR #202 and confirmed that removing in src/mcp/shared/session.py await self._incoming_message_stream_writer.send( notification ) will fix the probelm as well. So what does it tell us? It tells us that is that await self._incoming_message_stream_writer is blocking. Which in turn tells us that nobody ever reads from self._incoming_message_stream_reader. So where is self._incoming_message_stream_reader used? In BaseSession.incoming_messages. Now we have theory: incoming_messages is used to read incoming messages on the server. But it's in the BaseSession, so it means the client also offers it. Most clients don't read client.incoming_messages. IF clients don't read client.incoming_messages, the unbuffered stream never empties and once one object is placed, the next write will block. Now that we have the culprit we can figure out what to do. In my mind the probelm happens because we can't guarantee a consumer of incoming messages, however some implementations might want it. Similarly on the server side, incoming_messages is always used, since ServerSession is used inside Server. Okay so the goal was then to: Make consumption of incoming messages optionally for clients Make consumption of incoming message required for servers. So on the server side incoming_messages is fine, but the client needs a way to react if the developer is interested in. This means the approach of providing it via incoming_messages doesnt work. After thinking of ways to automatically drain, I came up what I think is best: The client offers a callback for all messages. This fits into the client interface using callbacks already. The server continues to expose incoming_messages. The BaseSession offers an abstraction for ClientSession and ServerSession to observe events from the _receive_loop but determine what to do by themselve.
Open Graph Description: This PR fixes a nasty but in the way we handle incoming messages, which currently leads to hanging connections when the clients sends notifications. The observed issue is as follows: A user create...
X Description: This PR fixes a nasty but in the way we handle incoming messages, which currently leads to hanging connections when the clients sends notifications. The observed issue is as follows: A user create...
Opengraph URL: https://github.com/modelcontextprotocol/python-sdk/pull/325
X: @github
Domain: github.com
| route-pattern | /:user_id/:repository/pull/:id/files(.:format) |
| route-controller | pull_requests |
| route-action | files |
| fetch-nonce | v2:cb6bc38d-9523-bf22-b56e-628433b60f0e |
| current-catalog-service-hash | ae870bc5e265a340912cde392f23dad3671a0a881730ffdadd82f2f57d81641b |
| request-id | E7D2:1CE497:B3C2B7:EF1FE3:6A5BBB1A |
| html-safe-nonce | 7dcc63267495599780564d04501ddc476fb1032e77736110517a97de206a3c00 |
| visitor-payload | eyJyZWZlcnJlciI6IiIsInJlcXVlc3RfaWQiOiJFN0QyOjFDRTQ5NzpCM0MyQjc6RUYxRkUzOjZBNUJCQjFBIiwidmlzaXRvcl9pZCI6IjY5MTU2MTAyNjEzNzU5MjUwMTgiLCJyZWdpb25fZWRnZSI6ImlhZCIsInJlZ2lvbl9yZW5kZXIiOiJpYWQifQ== |
| visitor-hmac | 13ce3a21f8cf238d2653f5b57a6ef87b0a14c85e61f967a259549c7f8bc91d8a |
| hovercard-subject-tag | pull_request:2405154897 |
| github-keyboard-shortcuts | repository,pull-request-list,pull-request-conversation,pull-request-files-changed,copilot |
| google-site-verification | Apib7-x98H0j5cPqHWwSMm6dNU4GmODRoqxLiDzdx9I |
| octolytics-url | https://collector.github.com/github/collect |
| analytics-location | / |
| fb:app_id | 1401488693436528 |
| apple-itunes-app | app-id=1477376905, app-argument=https://github.com/modelcontextprotocol/python-sdk/pull/325/files |
| twitter:image | https://avatars.githubusercontent.com/u/167242713?s=400&v=4 |
| twitter:card | summary_large_image |
| og:image | https://avatars.githubusercontent.com/u/167242713?s=400&v=4 |
| og:image:alt | This PR fixes a nasty but in the way we handle incoming messages, which currently leads to hanging connections when the clients sends notifications. The observed issue is as follows: A user create... |
| og:site_name | GitHub |
| og:type | object |
| hostname | github.com |
| expected-hostname | github.com |
| None | 5290d7e14309ad1e76106a9c4237bd1041517e83ea182c8ab756752cb0c6940b |
| turbo-cache-control | no-preview |
| diff-view | unified |
| go-import | github.com/modelcontextprotocol/python-sdk git https://github.com/modelcontextprotocol/python-sdk.git |
| octolytics-dimension-user_id | 182288589 |
| octolytics-dimension-user_login | modelcontextprotocol |
| octolytics-dimension-repository_id | 862584018 |
| octolytics-dimension-repository_nwo | modelcontextprotocol/python-sdk |
| octolytics-dimension-repository_public | true |
| octolytics-dimension-repository_is_fork | false |
| octolytics-dimension-repository_network_root_id | 862584018 |
| octolytics-dimension-repository_network_root_nwo | modelcontextprotocol/python-sdk |
| turbo-body-classes | logged-out env-production page-responsive full-width |
| disable-turbo | true |
| browser-stats-url | https://api.github.com/_private/browser/stats |
| browser-errors-url | https://api.github.com/_private/browser/errors |
| release | 9c975978430e9ad293956f2bbdaf153b1bd84a99 |
| ui-target | full |
| theme-color | #1e2327 |
| color-scheme | light dark |
Links:
Viewport: width=device-width