Skip to content
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

fix: re-use BufReader across poll_next invocations, instead of creating a new one #1093

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

kalvinnchau
Copy link
Collaborator

@kalvinnchau kalvinnchau commented Feb 5, 2025

  • re-use a single BufReader across poll_next calls, instead of creating a new one each time

    • this could cause buffered state to be lost across polls
  • also create a re-useable Vec that we read into, and use std::mem::take once we have a full line to read off

    • std::mem::take will also clear the vector

this hopefully adresses some odd timing issues we saw when communicating with mcp servers

we re-create the BufReader on each poll_next which might cause the
buffer state to be lost between polls
move BufReader to a field of ByteTransport and re-use the same BufReader
and see if buffer state is preserved
this avoids re-allocating the a new buffer, and having to regrow its
capacity
@salman1993
Copy link
Collaborator

cargo run -p mcp-client --example stdio_integration - failed for me

❯ cargo run -p mcp-client --example stdio_integration

...
2025-02-05T22:28:32.113621Z DEBUG mcp_client::transport::stdio: Stdout handler completed: ()
Error: McpServerError { method: "tools/list", server: "counter", source: ServerBoxError(Elapsed(())) }

@kalvinnchau
Copy link
Collaborator Author

cargo run -p mcp-client --example stdio_integration - failed for me

❯ cargo run -p mcp-client --example stdio_integration

...
2025-02-05T22:28:32.113621Z DEBUG mcp_client::transport::stdio: Stdout handler completed: ()
Error: McpServerError { method: "tools/list", server: "counter", source: ServerBoxError(Elapsed(())) }

hmmm can't replicate this error on my local machine or when running in a docker container

Copy link
Collaborator

@baxen baxen left a comment

Choose a reason for hiding this comment

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

Looks good! Let's leave a note imo on what we think this is about, something like

// The buf reader will accumulate from stdin or similar stream 
// we clear one line at each iteration of poll_next from the buffer 

#[pin]
writer: W,
buf: Vec<u8>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: if my understanding of the issue is correct, we don't need to keep this as a member and clear it below? seems simpler to instantiate a new one in each poll next instead. but let me know if i am missing something

Copy link
Collaborator

@salman1993 salman1993 left a comment

Choose a reason for hiding this comment

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

this works. i didn't pull the latest changes 🤦🏼

docs: add comments explaining why reader is BufReader
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants