Uh oh!
There was an error while loading. Please reload this page.
fix: make stdio shutdown more graceful - #364
Conversation
8159d42 to
da87299CompareUh oh!
There was an error while loading. Please reload this page.
jokemanfire
commented
Aug 12, 2025
I'm not sure if adding a shutdown interface is necessary, although it may increase some flexibility for the caller, it may be redundant for the SDK. For me, the previous direct killing behavior was not very elegant, and I would like to hear more opinions.How about you ? |
4t145
commented
Aug 12, 2025
I prefer to have a easier approch to cleanup in drop, and give user more flexibility in another explicit method. Or maybe we can just provide an into_inner method so the user can do anything they like with inner process. We can also wrap this wait-and-kill approch into a method, just as @crepererum metioned. |
jokemanfire
commented
Aug 13, 2025
Approve , I will change it. |
jokemanfire
commented
Aug 13, 2025
How about expose the transport in running service? @4t145 |
@jokemanfire Just have a look on the original issue
|
c494dbf to
9f38473Comparejokemanfire
commented
Aug 16, 2025
@4t145@crepererum Have a look , and pls check. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
4t145
commented
Aug 18, 2025
And what's your opinion about give back the ownership of transport after shutdown? Maybe we don't implement this feature in this pr, but it still a point to be discussed. |
According to the protocol specifications Signed-off-by: jokemanfire <hu.dingyang@zte.com.cn>
If we want to return ownership of the transport, I don't think it should be in the close interface. This can lead to misunderstandings in our understanding of the close interface. I suggest that if necessary, we can set up new interface extensions, and not every transport needs to implement ownership. I have added the into_inner interface for future extensions. |
4t145
commented
Aug 18, 2025
I mean, return it as serve task's result maybe change this pubasyncfnwaiting(self) -> Result<QuitReason, tokio::task::JoinError>to pubasyncfnwaiting(self) -> Result<(QuitReason,T), tokio::task::JoinError>But it will make more difficult to make |
Uh oh!
There was an error while loading. Please reload this page.
crepererum
commented
Aug 18, 2025
Thank you 🙂 |
- Add TransportWriter type alias for cleaner type definitions - Wrap transport writer in Option to enable proper closure - Implement close() method to drop writer and signal end of communication - Update graceful_shutdown to close transport before waiting for process exit - Improve error handling for closed transport state Addresses the graceful shutdown improvements discussed in modelcontextprotocol#347 and modelcontextprotocol#364
…392) - Add TransportWriter type alias for cleaner type definitions - Wrap transport writer in Option to enable proper closure - Implement close() method to drop writer and signal end of communication - Update graceful_shutdown to close transport before waiting for process exit - Improve error handling for closed transport state Addresses the graceful shutdown improvements discussed in #347 and #364
According to the protocol specifications Signed-off-by: jokemanfire <hu.dingyang@zte.com.cn>
…odelcontextprotocol#392) - Add TransportWriter type alias for cleaner type definitions - Wrap transport writer in Option to enable proper closure - Implement close() method to drop writer and signal end of communication - Update graceful_shutdown to close transport before waiting for process exit - Improve error handling for closed transport state Addresses the graceful shutdown improvements discussed in modelcontextprotocol#347 and modelcontextprotocol#364
According to the protocol specifications
Motivation and Context
Make shutdown more graceful.
How Has This Been Tested?
Ut
Breaking Changes
No breaking change
Types of changes
Checklist
Additional context