Fix serversocket addr race condition#928
Fix serversocket addr race condition#928AtharvaChiplunkar12 wants to merge 6 commits intouber:devfrom
Conversation
|
|
oneporter
left a comment
oneporter
left a comment
There was a problem hiding this comment.
I guess this was already fixed, can we pull the fixed version? https://github.com/apache/thrift/pull/3220/files
| p.mu.Lock() | ||
| listener := p.listener | ||
| p.mu.Unlock() |
There was a problem hiding this comment.
why take the write lock? Can we move line 79 to 71 and only take the read lock once?
There was a problem hiding this comment.
Yes, good point it will take a read lock. Since these changes were directly pulled from upstream, I assume this was overlooked there
| func (p *TServerSocket) Accept() (TTransport, error) { | ||
| p.mu.RLock() | ||
| interrupted := p.interrupted | ||
| listener := p.listener |
There was a problem hiding this comment.
In https://github.com/apache/thrift/blob/master/lib/go/thrift/server_socket.go, the listener is accessed under exclusive lock. Let's do the same here.
There was a problem hiding this comment.
it was a bug in upstream. fixed it on upstream apache/thrift@e7ab34e
| @@ -26,12 +26,12 @@ import ( | |||
| ) | |||
|
|
|||
| type TServerSocket struct { | |||
There was a problem hiding this comment.
Let's add more tests in server_socket_test.go and run them with race-detection flag
5 participants
Problem
The Addr() method in server_socket.go was accessing the p.listener field without mutex protection, which could cause data races when multiple goroutines access it concurrently (e.g., one goroutine calling Addr() while another calls Close(), Open(), or Listen()).
Solution
Added RLock()/RUnlock() protection around listener access for thread-safety
Changed direct nil check to use IsListening() method for consistency with the codebase
Follows the same locking pattern used in other methods (Accept(), Close(), Listen(), Open())
Sync with upstream fix apache/thrift@2620a12
Also, this PR includes all the necessary upstream fixes to ensure the file is up-to-date https://github.com/apache/thrift/blob/master/lib/go/thrift/server_socket.go