-
Notifications
You must be signed in to change notification settings - Fork 286
Closed
Labels
bugSomething isn't workingSomething isn't working
Milestone
Description
Describe the bug
The ServerSession.initialized method's current implementation calls ss.startKeepalive before checking if the session has already been initialized. This leads to startKeepalive being called on every initialized notification, regardless of the session state.
To Reproduce
func TestServerSessionkeepaliveCancelOverwritten(t *testing.T) {
server := NewServer(testImpl, &ServerOptions{KeepAlive: 5 * time.Second})
ss := &ServerSession{server: server}
// 1. Initialize the session.
_, err := ss.initialize(context.Background(), &InitializeParams{})
if err != nil {
t.Fatalf("ServerSession initialize failed: %v", err)
}
// 2. Call 'initialized' for the first time. This should start the keepalive mechanism.
_, err = ss.initialized(context.Background(), &InitializedParams{})
if err != nil {
t.Fatalf("First initialized call failed: %v", err)
}
if ss.keepaliveCancel == nil {
t.Fatalf("expected ServerSession.keepaliveCancel to be set after the first call of initialized")
}
firstCancel := ss.keepaliveCancel
defer firstCancel()
// 3. Manually set the field to nil.
// Do this to facilitate the test's core assertion. The goal is to verify that
// 'ss.keepaliveCancel' is not assigned a second time. By setting it to nil,
// we can easily check after the next call if a new keepalive goroutine was started.
ss.keepaliveCancel = nil
// 4. Call 'initialized' for the second time. This should return an error.
_, err = ss.initialized(context.Background(), &InitializedParams{})
if err == nil {
t.Fatalf("Expected 'duplicate initialized received' error on second call, got nil")
}
time.Sleep(2 * time.Second)
// 5. Re-check the field to ensure it remains nil.
// Since 'initialized' correctly returned an error and did not call
// 'startKeepalive', the field should remain unchanged.
if ss.keepaliveCancel != nil {
t.Fatal("expected ServerSession.keepaliveCancel to be nil after we manually niled it and re-initialized")
}
}
Expected behavior
ss.startKeepalive had better to be set exactly once as described in
Lines 870 to 880 in 1afdb1f
| func (ss *ServerSession) Close() error { | |
| if ss.keepaliveCancel != nil { | |
| // Note: keepaliveCancel access is safe without a mutex because: | |
| // 1. keepaliveCancel is only written once during startKeepalive (happens-before all 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 correctly | |
| ss.keepaliveCancel() | |
| } | |
| return ss.conn.Close() | |
| } |
Metadata
Metadata
Assignees
Labels
bugSomething isn't workingSomething isn't working