Dustin/138 deadlock remove global mutex #139
Conversation
WalkthroughThe pull request refactors the concurrency control in the Changes
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
server/server_race_test.go (1)
139-182:⚠️ Potential issueDeadlock-specific test scenario in
TestConcurrentPromptAdd.
- Adding a prompt from within another prompt’s handler is a classic scenario that can surface deadlocks.
- Using a goroutine within the handler to add a new prompt is an apt demonstration of nested concurrency.
- The test relies on
assert.Eventuallyto confirm the operation completes within a timeout, preventing indefinite hang.- Note that the error from
srv.handleGetPromptremains unchecked in test code (line 168).- srv.handleGetPrompt(ctx, "123", req) + if _, err := srv.handleGetPrompt(ctx, "123", req); err != nil { + t.Errorf("handleGetPrompt error: %v", err) + }🧰 Tools
🪛 golangci-lint (1.64.8)
168-168: Error return value of
srv.handleGetPromptis not checked(errcheck)
🧹 Nitpick comments (2)
server/server_race_test.go (1)
32-33: Test duration consideration.
A 300ms duration may be enough to catch short-lived races, but occasionally concurrency issues might show up under heavier load or longer runs. Consider parameterizing this duration or increasing it if race conditions persist in CI.server/server.go (1)
803-820: Thread-safe listing of tools inhandleListTools.
UsingtoolsMu.RLock/RUnlockis crucial. Sorting tool names outside of the lock’s scope is acceptable for large lists, but consider carefully if there's any risk of concurrent map modifications once you release the lock.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/server.go(18 hunks)server/server_race_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
server/server_race_test.go (3)
server/server.go (7)
NewMCPServer(375-401)WithPromptCapabilities(341-348)WithToolCapabilities(351-358)WithLogging(361-365)WithRecovery(318-329)ToolHandlerFunc(42-42)WithToolHandlerMiddleware(307-315)mcp/prompts.go (3)
Prompt(43-51)GetPromptRequest(20-28)GetPromptResult(32-37)mcp/tools.go (4)
Description(175-179)Tool(69-78)CallToolRequest(44-59)CallToolResult(34-41)
🪛 golangci-lint (1.64.8)
server/server_race_test.go
78-78: Error return value of srv.handleListTools is not checked
(errcheck)
82-82: Error return value of srv.handleListPrompts is not checked
(errcheck)
97-97: Error return value of srv.handleToolCall is not checked
(errcheck)
168-168: Error return value of srv.handleGetPrompt is not checked
(errcheck)
🔇 Additional comments (18)
server/server_race_test.go (11)
14-16: Good test documentation.
Clear explanation of the intent behind triggering race conditions. This helps future maintainers understand the purpose.
17-24: Solid approach to initializing the server with all capabilities.
This comprehensive setup ensures a broad coverage of concurrency interactions across all features.
26-30: Use of WaitGroup for concurrency is appropriate.
Combining aWaitGroupwith separate goroutines is a standard concurrency pattern and should work well for this test.
36-44: Concurrent addition of prompts.
Adding prompts while simultaneously performing other operations is a key scenario for verifying concurrency safety. The usage of a unique name for each prompt is a good way to avoid collisions.
46-54: Concurrent addition of tools.
This tests a parallel path analogous to prompt addition. Looks good and consistent overall.
56-66: Concurrent deletion of tools after addition.
Exercise thorough coverage by pairing add/delete operations. This is well-structured for testing potential race conditions with ephemeral entries.
68-75: Middleware addition in a concurrent context.
Testing concurrent modifications of thetoolHandlerMiddlewaresslice is crucial for verifying the separatemiddlewareMu. Good approach.
85-91: Persistent tool addition.
A persistent tool is a helpful test fixture for ensuring repeated calls remain valid. This approach is beneficial for real concurrency checks.
100-114: Concurrent resource addition.
Ensuring multiple resource additions do not cause data races is a good stress test for the newly introduced resource locks.
116-119: Final wait and log.
“No race conditions detected” logging is a clear test outcome indicator. Great finishing touch to summarize the test result.
121-137: Helper functionrunConcurrentOperation.
This is a clean abstraction, reducing duplication. Ensures each operation runs for the test duration, returning upon timeout.server/server.go (7)
144-151: Introduction of dedicated RWMutex fields.
Replacing a single global mutex with specialized locks is a solid concurrency design decision, reducing contention and lowering the risk of deadlocks. Great improvement.
311-315: Proper locking inWithToolHandlerMiddleware.
By lockingmiddlewareMubefore appending totoolHandlerMiddlewares, you avoid data races and preserve concurrency safety.
408-420: Granular locking inAddResource.
- Locking
capabilitiesMubefore checking resource capabilities handles concurrency changes in capabilities.- Locking
resourcesMuensures thread-safe access to the resources map.
427-439: Granular locking inAddResourceTemplate.
Similar pattern of locking for capabilities, followed by locking the resources map. This is consistent and safe.
443-453: Granular locking inAddPrompt.
This ensures safe concurrent registration of prompts and their handlers without blocking unrelated resource or tool operations.
462-476: Granular locking inAddTools.
Utilizes bothcapabilitiesMuandtoolsMuto safely register multiple tools concurrently. Sending a broadcast notification afterward is also a nice touch.
883-885: Thread-safe notification handlers.
ApplyingnotificationHandlersMu.RLock()/RUnlock()ensures simultaneous reads are safe. This finalizes the replacement of the global mutex with scoped locks.
This PR addresses #138. With the previous implementation the new tests will timeout due to the deadlock with the global mutex.
Summary by CodeRabbit