fix: add mutex to SSEServer to avoid data race between Start and Shutdown; fix test error on Windows (#166 #172) #170
Conversation
|
""" WalkthroughThis change introduces a read-write mutex to the Changes
Assessment against linked issues
Possibly related PRs
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 (
|
Wood-Q
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
server/sse.go (2)
212-212: Translate the Chinese comment to EnglishThe comment "关闭所有会话" should be translated to English (it means "close all sessions") to maintain consistency with the rest of the codebase.
- // 关闭所有会话 + // Close all active sessions
348-350: Unnecessary mutex usage with sync.MapThe sync.Map type is already designed to be safe for concurrent use without additional synchronization. The RLock/RUnlock around the sessions.Load call is unnecessary and could impact performance.
- s.mu.RLock() sessionI, ok := s.sessions.Load(sessionID) - s.mu.RUnlock()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/sse.go(4 hunks)
🔇 Additional comments (3)
server/sse.go (3)
69-69: Good addition of a mutex to prevent race conditionsAdding a sync.RWMutex to the SSEServer struct is an appropriate solution to protect the shared
srvfield from concurrent access between Start and Shutdown methods.
194-199: Proper mutex usage in Start methodThe write lock is correctly applied when assigning a new HTTP server instance to
s.srv. This prevents other goroutines from accessing or modifying the server during initialization.
207-211: Good implementation of read locking and local variable copyThe read lock is correctly used when accessing
s.srvand the pattern of copying to a local variable before unlocking is a best practice. This minimizes the lock duration while ensuring thread safety.
| keepAliveInterval time.Duration | ||
|
|
||
| mu sync.RWMutex |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Inconsistent protection pattern for the sessions map
While you've added mutex protection for the sessions map in the handleMessage method, the same protection is not applied in other methods that access this map (e.g., handleSSE, SendEventToSession). Either remove the mutex usage for sessions.Load (as sync.Map is already thread-safe) or consistently apply the protection pattern across all methods.
Also applies to: 212-219
🏁 Script executed:
#!/bin/bash
# Find all places where sessions map is accessed without mutex protection
echo "Checking for access to sessions map without mutex protection..."
rg -n "sessions\.(Load|Store|Delete|Range)" --type go server/sse.go | grep -v "mu\.(RLock|Lock)"Length of output: 502
Ensure consistent synchronization for the sessions map
We’ve found multiple s.sessions accesses that aren’t wrapped by your mu lock, yet you’ve also added locking in only some methods. Since you’re using a sync.Map (which is already safe for concurrent use), you should either:
• Remove the mu entirely and drop the extra locking around handleMessage, relying solely on sync.Map.
• Or keep mu and wrap all sessions.Load/Store/Delete/Range calls under mu.RLock/mu.Lock.
Unprotected accesses in server/sse.go:
- Line 213:
s.sessions.Range(...) - Line 217:
s.sessions.Delete(key) - Line 255:
s.sessions.Store(sessionID, session) - Line 256:
defer s.sessions.Delete(sessionID) - Line 349:
sessionI, ok := s.sessions.Load(sessionID) - Line 416:
sessionI, ok := s.sessions.Load(sessionID)
Please pick one synchronization pattern and apply it consistently to avoid races.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
client/transport/stdio_test.go (3)
310-310: Chinese comment should be translated to English.There is a Chinese comment in the codebase that should be translated to English for consistency throughout the codebase.
- // 创建一个新的 Stdio 实例但不调用 Start 方法 + // Create a new Stdio instance without calling the Start method
323-323: Chinese comment should be translated to English.There is another Chinese comment that should be translated to English for consistency.
- // 准备一个请求 + // Prepare a request
35-38: Consider refactoring repeated platform-specific code.The same pattern for adding the ".exe" suffix on Windows is duplicated three times in the code. Consider extracting this logic into a helper function to improve maintainability and follow the DRY principle.
+// getPlatformExecutablePath appends the .exe suffix when running on Windows +func getPlatformExecutablePath(basePath string) string { + if runtime.GOOS == "windows" { + return basePath + ".exe" + } + return basePath +}Then, use this helper function in each location:
- mockServerPath := filepath.Join(os.TempDir(), "mockstdio_server") - // Add .exe suffix on Windows - if runtime.GOOS == "windows" { - mockServerPath += ".exe" - } + mockServerPath := getPlatformExecutablePath(filepath.Join(os.TempDir(), "mockstdio_server"))Also applies to: 312-315, 343-346
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/transport/stdio_test.go(4 hunks)
🔇 Additional comments (4)
client/transport/stdio_test.go (4)
10-10: Runtime package import added for OS detection.The runtime package was added to enable the detection of the operating system, which is necessary for the platform-specific handling of executable filenames in this file.
35-38: Platform-specific handling for executable filename.These changes correctly add the ".exe" suffix to the executable filename when running on Windows, which is necessary for cross-platform compatibility.
312-315: Platform-specific handling for executable filename.These changes correctly add the ".exe" suffix to the executable filename when running on Windows.
343-346: Platform-specific handling for executable filename.These changes correctly add the ".exe" suffix to the executable filename when running on Windows.
Wood-Q
changed the title
server/sse.go
Outdated
| s.mu.RUnlock() | ||
|
|
||
| if srv != nil { | ||
| // 关闭所有会话 |
There was a problem hiding this comment.
Translate to English please :)
There was a problem hiding this comment.
sorry, I have deleted the annotation :)
What this PR does
SSEServer.StartandSSEServer.Shutdownby adding async.RWMutexto theSSEServerstruct.Startusesmu.Lockwhen settings.srvShutdownusesmu.RLockwhen accessings.srv'.exe'tomockServerPathRelated Issue
Closes #166
Closes #172
Summary by CodeRabbit
Summary by CodeRabbit