THRIFT-4237 Fix effective deadlock introduced by original patch #1304
Conversation
lib/go/thrift/server_socket.go
Outdated
| defer p.mu.RUnlock() | ||
| listener := p.listener | ||
| interrupted := p.interrupted | ||
| p.mu.RUnlock() | ||
|
|
There was a problem hiding this comment.
Hmm. It seems safest to me to unlock this mutex after the value of interrupted has been read (assuming it is only intended to protect interrupted).
For example,
Thread 1 reads the value of interrupted as false
Thread 1 gets interrupted at line 74
Thread 2 sets p.interrupted to true
Thread 1 sees interrupted == false at line 75
The question is would we want thread 1 to see interrupted == true ?
This is possibly a case that does not happen. I have very limited knowledge of what this code is doing.
There was a problem hiding this comment.
Agree. Looks strange. and the comment even says
// Protects the interrupted value to make it thread safe.
What about moving the listener := p.listener line down just before it is used? Or do I overlook sth?
There was a problem hiding this comment.
I will admit I also only have limited knowledge of what the code is doing. I am guessing as to the intended behavior based on the implementation. Is the expected behavior documented somewhere?
@econner This is something I considered, and here's what I reasoned out:
Ideally, we would ensure that the scenario you describe does not take place by continuing to hold the lock. In reality, the call to listener.Accept() may block, and if the lock was held this would prevent the Interrupt() method from acquiring the lock (resulting in the "deadlock" experienced previously). In the case that the instructions are interleaved as you describe, we will still return an error (albeit a different one) on line 84.
@Jens-G That does look viable to me.
Client: Go Patch: Zachary Wasserman <zachwass2000@gmail.com> This closes apache#1304
Client: Go Patch: Zachary Wasserman <zachwass2000@gmail.com> This closes apache#1304
Client: Go Patch: Zachary Wasserman <zachwass2000@gmail.com> This closes apache#1304
See discussion in https://issues.apache.org/jira/browse/THRIFT-4237