Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix:add read mutex in gettyWSConn(websocket) struct to prevent data race in ReadMessage() #83

Merged
merged 2 commits into from
Jun 5, 2024

Conversation

No-SilverBullet
Copy link
Collaborator

What this PR does:

  1. rename "lock" to "writeLock" and add "readLock" in gettyWSConn
  2. use the "readLock" to wrap the websocket'sReadMessage function to be concurrency safe(
    similar to WriteMessage())

Which issue(s) this PR fixes:

NONE
Special notes for your reviewer:
NONE
Does this PR introduce a user-facing change?:

NONE

No-SilverBullet and others added 2 commits June 5, 2024 16:11
…ace in ReadMessage().

>
>
Co-authored-by: No-SilverBullet <13708123240@163.com>
Co-authored-by: AlexStocks <alexstocks@foxmail.com>
…ace in ReadMessage().

Co-authored-by: AlexStocks <alexstocks@foxmail.com>
@No-SilverBullet
Copy link
Collaborator Author

similar with downstream repo dubbo-getty's pr( apache#123)

@AlexStocks AlexStocks merged commit da9fedf into master Jun 5, 2024
1 check passed
@AlexStocks
Copy link
Owner

@CodiumAI-Agent /review

@CodiumAI-Agent
Copy link

PR Review 🔍

⏱️ Estimated effort to review [1-5]

2, because the changes are straightforward and localized to specific functions within a single file. The renaming of variables and the addition of a new mutex are clear and do not involve complex logic changes.

🧪 Relevant tests

No

⚡ Possible issues

Possible Bug: The new threadSafeReadMessage function locks and unlocks readLock but does not handle potential panics which could occur before the unlock. This might leave the mutex locked indefinitely, leading to deadlock situations.

🔒 Security concerns

No

Code feedback:
relevant filetransport/connection.go
suggestion      

Consider using defer to unlock the readLock immediately after locking to ensure the lock is released even if a panic occurs. This change enhances the robustness of the concurrency control by preventing deadlocks in case of unexpected panics. [important]

relevant linew.readLock.Lock()

@AlexStocks
Copy link
Owner

@CodiumAI-Agent /improve

@CodiumAI-Agent
Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Best practice
Improve error handling in the CloseConn method

Consider handling the error from w.conn.Close() in the CloseConn method to ensure proper
error handling and resource cleanup.

transport/connection.go [644]

-w.conn.Close()
+if err := w.conn.Close(); err != nil {
+    // Handle error, possibly log it or return it
+}
 
Suggestion importance[1-10]: 9

Why: This suggestion improves error handling by ensuring that any errors from w.conn.Close() are properly handled, which is crucial for resource cleanup and debugging.

9
Possible issue
Modify locking strategy to handle errors before deferring unlocks

To avoid potential deadlocks and improve error handling, consider checking for errors
immediately after locking and before deferring the unlock in threadSafeWriteMessage and
threadSafeReadMessage.

transport/connection.go [649-653]

 w.writeLock.Lock()
-defer w.writeLock.Unlock()
 if err := w.conn.WriteMessage(messageType, data); err != nil {
+    w.writeLock.Unlock()  // Unlock before handling error
     return err
 }
+w.writeLock.Unlock()
 
Suggestion importance[1-10]: 8

Why: This suggestion addresses a potential deadlock issue by ensuring that the lock is released before handling errors, which improves the robustness of the code.

8
Enhancement
Use sync.RWMutex for more efficient read/write locking

Instead of using separate writeLock and readLock, consider using a single sync.RWMutex
which can be more efficient if reads are more frequent than writes.

transport/connection.go [496-497]

-writeLock sync.Mutex
-readLock  sync.Mutex
+connLock sync.RWMutex
 
Suggestion importance[1-10]: 7

Why: Using sync.RWMutex can improve efficiency if reads are more frequent than writes, but this change is more of an optimization and not critical for functionality.

7
Possible bug
Add error handling for potential overflow in byte count addition

Add error handling for w.readBytes.Add((uint32)(len(b))) to ensure that the operation does
not result in unexpected behavior or overflow.

transport/connection.go [575]

-w.readBytes.Add((uint32)(len(b)))
+if len(b) > math.MaxUint32 {
+    // Handle error or overflow case
+} else {
+    w.readBytes.Add((uint32)(len(b)))
+}
 
Suggestion importance[1-10]: 6

Why: While adding error handling for potential overflow is a good practice, the likelihood of encountering an overflow in this context is low. This makes the suggestion less critical but still beneficial for robustness.

6

@AlexStocks AlexStocks deleted the fix/ws-concurrent-read branch June 5, 2024 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants