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

sync.Cond.Signal() does not respect wait generations #1648

Closed
dvyukov opened this issue Mar 28, 2011 · 10 comments
Closed

sync.Cond.Signal() does not respect wait generations #1648

dvyukov opened this issue Mar 28, 2011 · 10 comments

Comments

@dvyukov
Copy link
Member

dvyukov commented Mar 28, 2011

>What steps will reproduce the problem?
Here is a reproducer:

Mutex mtx;
Cond cv;
int state; // = 0

//thread 0
mtx.lock();
while (state == 0)
  cv.wait();
state = 2;
cv.signal();
mtx.unlock();

//thread 1
mtx.lock();
state = 1;
cv.signal();
mtx.unlock();

//thread 2
for (;;) {
  mtx.lock();
  if (state != 0) {
    while (state != 2)
      cv.wait();
  }
  mtx.unlock();
}

Signal from thread 1 must be caught by thread 0, because it's only it that possibly
waits on the cv while state==0. However, AFAIS the signal can be caught by thread 2.

>What is the expected output?
Program does not deadlock.

>What do you see instead?
Program sometimes deadlocks.

>Which compiler are you using (5g, 6g, 8g, gccgo)?
Does not have a compiler yet.

>Which operating system are you using?
-

>Which revision are you using?  (hg identify)
fad73d342108 tip

>Please provide any additional information below.
It's only my mental experiment. Installation and studying of Go will take some time, but
I don't want the bug to lost.
@adg
Copy link
Contributor

adg commented Mar 31, 2011

Comment 1:

Owner changed to @rsc.

Status changed to Accepted.

@niemeyer
Copy link
Contributor

Comment 2:

This is working as intended.  Just don't use Signal, because you can't tell which
goroutine it will awake.  Use Broadcast instead.

Status changed to WorkingAsIntended.

@dvyukov
Copy link
Member Author

dvyukov commented May 31, 2011

Comment 3:

There is only 1 goroutine waits on the cv, so it's pretty trivial to tell which
goroutine signal must awake.

@niemeyer
Copy link
Contributor

Comment 4:

Here is the order of events for getting both locked up:
A) Thread 0 executes, stops in cv.wait()
B) Thread 1 executes, sets state to 1, calls cv.signal()
C) Thread 2 executes, stops in cv.wait()
D) Thread 2 unblocks, due to signal from (B)
E) Thread 2 loops, blocks again on cv.wait()
Deadlock, with two goroutines dead.  In this case, not even Broadcast will help.
There's actually an improvement we can make to avoid (C) happening when Signal() takes
the number of goroutines from 1 to 0, but in the end it still boils down to the
expectation of which goroutine Signal() is awakening.
That said, please post the snippet you have with real code.  I'll have a look at it.

Owner changed to @niemeyer.

Status changed to Thinking.

@niemeyer
Copy link
Contributor

Comment 5:

Actually, I lied.  Using Broadcast would still solve the problem in this case,
because it's a real barrier that unblocks all existent goroutines.

@dvyukov
Copy link
Member Author

dvyukov commented May 31, 2011

Comment 6:

Here is the repro.
Please put it into src/sync/cond_test.go after fixing the issue.
import (
    . "sync"
    "testing"
    "runtime"
)
func TestCondWaitGenerations(t *testing.T) {
    procs := runtime.GOMAXPROCS(4)
    for i := 0; i < 1000; i ++ {
        var m Mutex
        c := NewCond(&m)
        state := 0
        go func() {
            m.Lock();
            for state == 0 {
                c.Wait()
            }
            state = 2
            c.Signal()
            m.Unlock()
        }()
        go func() {
            m.Lock()
            state = 1
            c.Signal()
            m.Unlock()
        }()
        go func() {
            for true {
                m.Lock()
                if (state != 0) {
                    for state != 2 {
                        c.Wait()
                    }
                } else {
                    break
                }
                m.Unlock()
            }
        }()
    }
    runtime.GOMAXPROCS(procs)
}

@dvyukov
Copy link
Member Author

dvyukov commented May 31, 2011

Comment 7:

> Here is the order of events for getting both locked up:
Indeed. That's what I meant. It should not happen.
"Signal wakes one goroutine waiting on c, if there is any.". So C) should be Thread 0
unblocks.

@niemeyer
Copy link
Contributor

Comment 8:

Fair enough, I'll fix that.

Status changed to Accepted.

@niemeyer
Copy link
Contributor

Comment 9:

The fix is up for review, with a simpler test case:
    http://golang.org/cl/4524083
Note that your example above has a bug: it's breaking out of the
for loop with a mutex held.

Status changed to Started.

@niemeyer
Copy link
Contributor

niemeyer commented Jun 1, 2011

Comment 10:

This issue was closed by revision 17bfa32.

Status changed to Fixed.

@golang golang locked and limited conversation to collaborators Jun 24, 2016
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants