You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by zwass <gi...@git.apache.org> on 2017/07/07 00:40:56 UTC
[GitHub] thrift pull request #1304: THRIFT-4237 Fix effective deadlock introduced by ...
GitHub user zwass opened a pull request:
https://github.com/apache/thrift/pull/1304
THRIFT-4237 Fix effective deadlock introduced by original patch
See discussion in https://issues.apache.org/jira/browse/THRIFT-4237
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zwass/thrift THRIFT-4237-fix
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1304.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1304
----
commit 6e9d0e79a98e62aef86aaada2da7fc0e04795d28
Author: Zachary Wasserman <za...@gmail.com>
Date: 2017-07-07T00:39:55Z
THRIFT-4237 Fix effective deadlock introduced by original patch
See discussion in https://issues.apache.org/jira/browse/THRIFT-4237
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1304: THRIFT-4237 Fix effective deadlock introduced by ...
Posted by econner <gi...@git.apache.org>.
Github user econner commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1304#discussion_r126207049
--- Diff: lib/go/thrift/server_socket.go ---
@@ -68,15 +68,18 @@ func (p *TServerSocket) Listen() error {
func (p *TServerSocket) Accept() (TTransport, error) {
p.mu.RLock()
- defer p.mu.RUnlock()
+ listener := p.listener
+ interrupted := p.interrupted
+ p.mu.RUnlock()
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1304: THRIFT-4237 Fix effective deadlock introduced by ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/1304
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1304: THRIFT-4237 Fix effective deadlock introduced by ...
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1304#discussion_r126221534
--- Diff: lib/go/thrift/server_socket.go ---
@@ -68,15 +68,18 @@ func (p *TServerSocket) Listen() error {
func (p *TServerSocket) Accept() (TTransport, error) {
p.mu.RLock()
- defer p.mu.RUnlock()
+ listener := p.listener
+ interrupted := p.interrupted
+ p.mu.RUnlock()
--- End diff --
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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1304: THRIFT-4237 Fix effective deadlock introduced by ...
Posted by zwass <gi...@git.apache.org>.
Github user zwass commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1304#discussion_r126230574
--- Diff: lib/go/thrift/server_socket.go ---
@@ -68,15 +68,18 @@ func (p *TServerSocket) Listen() error {
func (p *TServerSocket) Accept() (TTransport, error) {
p.mu.RLock()
- defer p.mu.RUnlock()
+ listener := p.listener
+ interrupted := p.interrupted
+ p.mu.RUnlock()
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---