fix: don't share mutex between tools/resources #574
Conversation
I noticed that mark3labs#569 was merged (thanks!) and looked at the diff again with fresh eyes and noticed that I reused the existing mutex for tool middlewares within the resource middlewares. This means that, at least while processing middlewares, it's possible a resource call could be blocked waiting on a lock because of a tool call or vice-versa. Since there's a separate mutex for tools, resources, etc, it seems there's a desire to not block each other. This commit renames the existing middleware mutex to better clarify it's specifically for tool middlewares, and adds a new mutex for use specifically with resource middlewares. Signed-off-by: TJ Hoplock <[email protected]>
WalkthroughIntroduces per-type middleware locking in MCPServer by replacing a single mutex with separate RWMutexes for resource and tool middlewares, updating registration functions to use these locks, and applying read locks during middleware invocation in handleReadResource and handleToolCall. Struct fields updated accordingly; initialization otherwise unchanged. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 0
🧹 Nitpick comments (2)
server/server.go (2)
880-886: Snapshot middlewares under lock before applying them.Take a shallow copy while holding RLock so later in-place mutations (should they be introduced) can’t race with iteration here.
Apply:
- s.resourceMiddlewareMu.RLock() - mw := s.resourceHandlerMiddlewares + s.resourceMiddlewareMu.RLock() + mw := append([]ResourceHandlerMiddleware(nil), s.resourceHandlerMiddlewares...) // Apply middlewares in reverse order for i := len(mw) - 1; i >= 0; i-- { finalHandler = mw[i](finalHandler) } - s.resourceMiddlewareMu.RUnlock() + s.resourceMiddlewareMu.RUnlock()Also, confirm whether resource middlewares are intentionally not applied to template handlers below (matchedHandler path). If they should be, we can adapt by wrapping a ResourceHandlerFunc adapter around matchedHandler.
1142-1149: Do the same snapshot for tool middlewares.Mirror the resource change to avoid any future in-place edits racing with reads.
Apply:
- s.toolMiddlewareMu.RLock() - mw := s.toolHandlerMiddlewares + s.toolMiddlewareMu.RLock() + mw := append([]ToolHandlerMiddleware(nil), s.toolHandlerMiddlewares...) // Apply middlewares in reverse order for i := len(mw) - 1; i >= 0; i-- { finalHandler = mw[i](finalHandler) } - s.toolMiddlewareMu.RUnlock() + s.toolMiddlewareMu.RUnlock()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
server/server.go(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-23T11:10:42.948Z
Learnt from: floatingIce91
PR: mark3labs/mcp-go#401
File: server/server.go:1082-1092
Timestamp: 2025-06-23T11:10:42.948Z
Learning: In Go MCP server, ServerTool.Tool field is only used for tool listing and indexing, not for tool execution or middleware. During handleToolCall, only the Handler field is used, so dynamic tools don't need the Tool field populated.
Applied to files:
server/server.go
🔇 Additional comments (3)
server/server.go (3)
225-228: Correct: exclusive lock around append to tool middlewares.Write-side is properly synchronized.
237-240: Correct: exclusive lock around append to resource middlewares.Write-side is properly synchronized.
150-150: No leftovermiddlewareMureferences; split locks confirmed safe.
2 participants
I noticed that #569 was merged (thanks!) and looked at the diff again
with fresh eyes and noticed that I reused the existing mutex for tool
middlewares within the resource middlewares. This means that, at least
while processing middlewares, it's possible a resource call could be
blocked waiting on a lock because of a tool call or vice-versa.
Since there's a separate mutex for tools, resources, etc, it seems
there's a desire to not block each other. This commit renames the
existing middleware mutex to better clarify it's specifically for tool
middlewares, and adds a new mutex for use specifically with resource
middlewares.
Signed-off-by: TJ Hoplock [email protected]
Summary by CodeRabbit