-
Notifications
You must be signed in to change notification settings - Fork 266
mcp: Implement KeepAlive for client and server #39
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -308,3 +308,41 @@ type listResult[T any] interface { | |
| // Returns a pointer to the param's NextCursor field. | ||
| nextCursorPtr() *string | ||
| } | ||
|
|
||
| // keepaliveSession represents a session that supports keepalive functionality. | ||
| type keepaliveSession interface { | ||
| Ping(ctx context.Context, params *PingParams) error | ||
| Close() error | ||
| } | ||
|
|
||
| // startKeepalive starts the keepalive mechanism for a session. | ||
| // It assigns the cancel function to the provided cancelPtr and starts a goroutine | ||
| // that sends ping messages at the specified interval. | ||
| func startKeepalive(session keepaliveSession, interval time.Duration, cancelPtr *context.CancelFunc) { | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| // Assign cancel function before starting goroutine to avoid race condition. | ||
| // We cannot return it because the caller may need to cancel during the | ||
| // window between goroutine scheduling and function return. | ||
| *cancelPtr = cancel | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a comment saying that we must assign this before starting the goroutine, so we cannot return it. (I first thought "why not just return the cancel func instead of passing in a pointer?" But it would be racy.)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good suggestion, thank you! Updated with this comment: // Assign cancel function before starting goroutine to avoid race condition.
// We cannot return it because the caller may need to cancel during the
// window between goroutine scheduling and function return. |
||
|
|
||
| go func() { | ||
| ticker := time.NewTicker(interval) | ||
| defer ticker.Stop() | ||
|
|
||
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return | ||
| case <-ticker.C: | ||
| pingCtx, pingCancel := context.WithTimeout(context.Background(), interval/2) | ||
| err := session.Ping(pingCtx, nil) | ||
| pingCancel() | ||
| if err != nil { | ||
| // Ping failed, close the session | ||
| _ = session.Close() | ||
| return | ||
| } | ||
| } | ||
| } | ||
| }() | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like a user Close and the Close from the startKeepalive goroutine could race.
I don't think they can, but it's worth an explanation, in terms of happens-before relations, why no mutex is needed for keepaliveCancel. (If you don't understand what I'm talking about, I can explain further.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya, I agree, I'm pretty sure that we do not need a mutex here, I've added the following comment explaining the flow and why it's safe:
Please double check my reasoning though!