Lock main client from inside the session client when session requests are made#71
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request addresses a race condition in token refresh when session requests are made from MessageSessionClient. The fix ensures that the parent Client's mutex is held when session requests invoke newActionsServiceRequest, which may trigger token refresh operations that modify shared client state.
Changes:
- Lock the main client mutex in
MessageSessionClient.doSessionRequestbefore calling methods that may update token state - Unlock the client mutex earlier in
MessageSessionClientcreation to avoid holding the lock during the initial session creation network call - Enable race detection in tests by adding the
-raceflag to the go test command
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| session_client.go | Added locking of innerClient.mu in doSessionRequest to prevent concurrent token updates |
| client.go | Moved the unlock to occur before createMessageSession is called to avoid holding the lock during network I/O in the creation phase |
| .github/workflows/go.yaml | Added -race flag to test execution to detect race conditions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| c.innerClient.mu.Lock() | ||
| defer c.innerClient.mu.Unlock() |
There was a problem hiding this comment.
The mutex is held during HTTP I/O operations (lines 247-263), which could cause performance degradation in high-concurrency scenarios. While this pattern is consistent with how other Client methods work throughout the codebase, holding a mutex during network I/O can lead to head-of-line blocking where all operations on the parent Client are blocked while a session request is in progress.
However, this is necessary to prevent race conditions in updateTokenIfNeeded. Consider documenting this performance characteristic in the MessageSessionClient documentation, noting that session requests will serialize with other Client operations. Also consider whether future optimizations might use a more fine-grained locking strategy (e.g., separate locks for token state vs. other Client state).
Fixes #56