chore: Add AsyncCloseable interface.#95
chore: Add AsyncCloseable interface.#95He-Pin wants to merge 1 commit intomodelcontextprotocol:mainfrom
Conversation
| * Immediately closes the resource gracefully. | ||
| */ | ||
| default void close() { | ||
| this.closeGracefully().subscribe(); |
There was a problem hiding this comment.
Maybe the default implementation should call block
chemicL
left a comment
chemicL
left a comment
There was a problem hiding this comment.
Thank you for the PR. I think with a few tweaks it will be a useful addition.
|
|
||
| /** | ||
| * Begins the process of closing the resource gracefully, if there is one in progress, | ||
| * return the existing one. |
There was a problem hiding this comment.
I don't think that's accurate? We'd need to keep track of closing-in-progress and I am not certain we do that currently.
There was a problem hiding this comment.
So you suggest we drop the :
if there is one in progress, return the existing one.
?
I think we should always return the same instance.
There was a problem hiding this comment.
I checked the code, you are right, not all implementation return the same Mono, will drop this description.
| Mono<Void> closeGracefully(); | ||
|
|
||
| /** | ||
| * Immediately closes the resource gracefully. |
There was a problem hiding this comment.
The graceful close is not immediate, it kicks off asynchronously. In the cases of void return type it is a disconnected chain.
| * Immediately closes all the transports with connected clients and releases any | ||
| * associated resources. | ||
| */ | ||
| @Override |
|
|
||
| /** | ||
| * Immediately closes the resource gracefully. | ||
| */ |
There was a problem hiding this comment.
An @see #closeGracfully() javadoc entry would be useful.
| /** | ||
| * Closes the session and releases any associated resources. | ||
| */ | ||
| @Override |
| * needed. It should handle the graceful shutdown of any active connections. | ||
| * </p> | ||
| */ | ||
| @Override |
He-Pin
force-pushed
the
asyncCloseable
branch
2 times, most recently
from
c8a8746 to
66f6e35
Compare
Signed-off-by: He-Pin <hepin1989@gmail.com>
| }); | ||
| } | ||
| mcpServer.closeGracefully().block(); | ||
| mcpServer.closeGracefully(); |
There was a problem hiding this comment.
This won't close the server since the Mono is not subscribed to. As this is an async server it is required to subscribe. I think the same issue has been introduced in multiple other places.
- Please review all the code changes for this error.
- Please do not force-push as it makes reviewing difficult.
Thanks!
There was a problem hiding this comment.
- We should always return the same instance.
- We should do the close/subscribe when invoking.
But , these are not consistent across the codebase.
In akka/pekko, there is a whenTerminate, which will return the same instance too.
There was a problem hiding this comment.
wdyt about this @chemicL , I'm a little busy at work, but I can update this pr with your input.
There was a problem hiding this comment.
I'm not sure - is this in reference to my comment above? I only wanted to indicate that the mcpServer instance is an McpAsyncServer, which means closeGracefully() returns a Mono, which needs to be subscribed to in order to trigger closing, otherwise nothing happens.
That comment does not touch upon returning the same instance of Mono, just the fact that it won't close anything.
For returning the same instance - I am not sure whether we need that - do you think we do? I imagine in most cases the double closing should be idempotent so perhaps there is no need. If we do have issues with double closing, let's open a new issue to address this and think of solutions, WDYT?
2 participants
Motivation and Context
Introduce a common interface for autocloseable.
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context