Skip to content

Cleans read stream writers list#893

Closed
rahulchittimalla wants to merge 1 commit intomodelcontextprotocol:mainfrom
rahulchittimalla:cleanup/read-steam-writers
Closed

Cleans read stream writers list#893
rahulchittimalla wants to merge 1 commit intomodelcontextprotocol:mainfrom
rahulchittimalla:cleanup/read-steam-writers

Conversation

@rahulchittimalla
Copy link
Copy Markdown

@rahulchittimalla rahulchittimalla commented Jun 6, 2025 •
edited
Loading

Motivation and Context

Currently, there is no explicit cleanup of the session_id from the _read_stream_writers. When the connection closes (client disconnects) the code closes the streams, but does not remove the session_id key from _read_stream_writers dictionary when the connection is closed.

How Has This Been Tested?

Breaking Changes

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

felixweinberger
felixweinberger requested changes Sep 17, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@felixweinberger felixweinberger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @rahulchittimalla - thank you for your contribution! And apologies for the delay in getting back to you.

Could you provide some additional detail what problem you were running into without this? I understand the session_id not being removed from the _read_stream_writers but I'm curious whether this was actually causing any issues for you?

If this was causing bugs in production for you or if there's some reproducible problem as a result of this, would be great to add a test that demonstrates the broken behavior?

Happy to get back to this quickly if you have the bandwidth to update the PR here!

@felixweinberger felixweinberger added needs confirmation Needs confirmation that the PR is actually required or needed. needs more work Not ready to be merged yet, needs additional follow-up from the author(s). labels Sep 17, 2025
@felixweinberger
Copy link
Copy Markdown
Contributor

felixweinberger commented Sep 26, 2025

Closing this one for now as it's been a while - we're currently going through older PRs to ensure we prioritize limited maintainer time appropriately.

Happy to reopen this if you come back to this at a later time to continue the discussion - in that case feel free to ping here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@felixweinberger felixweinberger felixweinberger requested changes

Assignees

No one assigned

Labels

needs confirmation Needs confirmation that the PR is actually required or needed. needs more work Not ready to be merged yet, needs additional follow-up from the author(s).

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@rahulchittimalla @felixweinberger