AlexStocks / getty

a netty like asynchronous network I/O library based on tcp/udp/websocket; a bidirectional RPC framework based on JSON/Protobuf; a microservice framework based on zookeeper/etcd
Apache License 2.0
888 stars 186 forks source link

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

Closed No-SilverBullet closed 4 months ago

No-SilverBullet commented 4 months ago

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 commented 4 months ago

similar with downstream repo dubbo-getty's pr( https://github.com/apache/dubbo-getty/pull/123)

AlexStocks commented 4 months ago

@CodiumAI-Agent /review

CodiumAI-Agent commented 4 months ago

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 commented 4 months ago

@CodiumAI-Agent /improve

CodiumAI-Agent commented 4 months ago

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]](https://github.com/AlexStocks/getty/pull/83/files#diff-f5c4aeed2bc9e490b90f0afe45338705de3c781ee30910959d5671b3981ccb55R644-R644) ```diff -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]](https://github.com/AlexStocks/getty/pull/83/files#diff-f5c4aeed2bc9e490b90f0afe45338705de3c781ee30910959d5671b3981ccb55R649-R653) ```diff 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]](https://github.com/AlexStocks/getty/pull/83/files#diff-f5c4aeed2bc9e490b90f0afe45338705de3c781ee30910959d5671b3981ccb55R496-R497) ```diff -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]](https://github.com/AlexStocks/getty/pull/83/files#diff-f5c4aeed2bc9e490b90f0afe45338705de3c781ee30910959d5671b3981ccb55R575-R575) ```diff -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