-
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
mcp: Implement KeepAlive for client and server #39
Conversation
| // from being handled, and waiting for ongoing requests to return. Close then | ||
| // terminates the connection. | ||
| func (cs *ClientSession) Close() error { | ||
| if cs.keepaliveCancel != nil { |
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:
// Note: keepaliveCancel access is safe without a mutex because:
// 1. keepaliveCancel is only written once during startKeepalive (happens-before user Close calls)
// 2. context.CancelFunc is safe to call multiple times and from multiple goroutines
// 3. The keepalive goroutine calls Close on ping failure, but this is safe since
// Close is idempotent and conn.Close() handles concurrent calls correctlyPlease double check my reasoning though!
30d2449 to
1d577ce
Compare
| // that sends ping messages at the specified interval. | ||
| func startKeepalive(session keepaliveSession, interval time.Duration, cancelPtr *context.CancelFunc) { | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| *cancelPtr = cancel |
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.
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.)
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.
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.1d577ce to
f3f9a24
Compare
samthanawalla
left a comment
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.
LGTM
Motivation and Context
Implement
KeepAlivefeatures from the design docCloses #24
How Has This Been Tested?
Tested locally (unit tests).
Breaking Changes
N/A
Types of changes
Checklist
Additional context
I can send a follow-up PR adding an example with a StreamableHTTP server that uses the keepalive if that would be helpful.