Does the Wait function of sync.WaitGroup behave as a memory barrier?

[kai...@gmail.com]

Here is some example code that demonstrates my question:

var condition bool

var wg sync.WaitGroup

for _, item := range items {

wg.Add(1)

go func(item) {

if meetsCondition(item) {

condition = true

}

wg.Done()

}(item)

}

wg.Wait()

// is it safe to check condition here?

Does wg.Wait() act as a memory barrier so that it's safe to check condition? If not, what's the best way to represent this idiom? (atomic.Load/Store?) Thanks!

[Ian Lance Taylor]

It is safe to check condition after wg.Wait returns.

On the other hand you have several goroutines racing to set condition,
so you should be using atomic.Store anyhow.

Or just send a value on a channel, that always works and doesn't lead
to any of these issues. Untested:

var wg sync.WaitGroup
c := make(chan bool, 1)

for _, item := range items {
wg.Add(1)
go func(item) {
if meetsCondition(item) {

select {
case c <- true:
default:
}
}
wg.Done()
}(item)
}
wg.Wait()
close(c)
condition, _ := <-c

Ian

[Kai Skye]

Well, I figure that the race condition is benign here, since even if a goroutine sees an old value, all I care about is that by the time wg.Wait() is finished, condition is either set or unset. But please correct me if I'm wrong about that.

I like the idea of using channels but I couldn't figure out how to get it playing nicely with WaitGroup. In your example, you do:

wg.Wait()

close(c)

condition, _ := <-c

But I am under the impression that reading from a closed channel always returns a zero value, so condition would always be false in this case. Is that not true here? Otherwise, if we don't close the channel and just try to do "condition := <-c", then it would hang if "c <- true" is never called by any goroutine. Although, perhaps I could use a select statement after wg,Wait(), as in:

wg.Wait()

select {

    case <-c:

        // do stuff with condition == true

    default:

        // do other stuff

}

Does that seem reasonable?

[Thomas Bushnell, BSG]

If two goroutines, write the same variable concurrently without any protection, you cannot be guaranteed at the end that the value is one or the other value.

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Kai Skye]

If two goroutines, write the same variable concurrently without any protection, you cannot be guaranteed at the end that the value is one or the other value.

That's certainly true if multiple goroutines are attempting to write different values to the same location. But I think that in this case the race is benign because it's many goroutines all competing to write the same value. So if I have the value "false" stored initially, and 100 different goroutines compete to write the value "true", I don't really care which one wins as long as the value is "true" by the end. As long as the read happens after a memory barrier, I can't think of a situation where that could fail. But again, please correct me if I'm wrong.

[Ian Lance Taylor]

On Tue, May 6, 2014 at 6:34 PM, Kai Skye <kai...@gmail.com> wrote:
>

> Well, I figure that the race condition is benign here, since even if a
> goroutine sees an old value, all I care about is that by the time wg.Wait()
> is finished, condition is either set or unset. But please correct me if I'm
> wrong about that.

In general all races should be avoided. I agree that this one is
likely benign.

> I like the idea of using channels but I couldn't figure out how to get it
> playing nicely with WaitGroup. In your example, you do:
>
> wg.Wait()
> close(c)
> condition, _ := <-c
>
> But I am under the impression that reading from a closed channel always
> returns a zero value, so condition would always be false in this case. Is
> that not true here?

A close of a channel is in effect sent on the channel. The close will
only be seen after all other values have been read. So in this case
if any goroutine wrote true to the channel, that is the value that
will wind up in the variable "condition". If no goroutine wrote to
the channel, then it will be closed, and "condition" will be set to
the zero value == false.

In fact I now see that the _ is pointless and it would fine to write
simply
condition := <-c

> Otherwise, if we don't close the channel and just try to
> do "condition := <-c", then it would hang if "c <- true" is never called by
> any goroutine. Although, perhaps I could use a select statement after
> wg,Wait(), as in:
>
> wg.Wait()
> select {
> case <-c:
> // do stuff with condition == true
> default:
> // do other stuff
> }
>
> Does that seem reasonable?

Yes, that will also work, it's just more verbose.

Ian

On Tue, May 6, 2014 at 6:34 PM, Kai Skye <kai...@gmail.com> wrote:

[David Anderson]

An interesting perspective on "benign" data races: https://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong . It makes a good case for not standing for races that are "probably okay", because hidden assumptions can mess with you in very subtle ways down the road.

- Dave

[Dmitry Vyukov]

wg.Add happens-before wg.Wait

You need to protect accesses to condition.
Either change it to uint32 and use atomic.StoreUint32 or aggregate
results with a channel:

done := make(chan bool)

for _, item := range items {

go func(item) {
done <- meetsCondition(item)
}(item)
}
cond := false
for _ := range items {
cond = cond | <-done

[Rui Ueyama]

On Tue, May 6, 2014 at 10:45 PM, 'Dmitry Vyukov' via golang-nuts <golan...@googlegroups.com> wrote:

wg.Add happens-before wg.Wait

Is it documented?

[Dmitry Vyukov]

no
there is an issue for sync/atomic:
https://code.google.com/p/go/issues/detail?id=5045
I've filed a similar one for sync:
https://code.google.com/p/go/issues/detail?id=7948

[Nico]

On 07/05/14 07:17, 'Dmitry Vyukov' via golang-nuts wrote:
> no
> there is an issue for sync/atomic:
> https://code.google.com/p/go/issues/detail?id=5045

Dmitry,

in the second of the litmus tests you mentioned above:

2.

// goroutine 1
atomic.Store(&X, 1)
r1 = atomic.Load(&Y)

// goroutine 2
atomic.Store(&Y, 1)
r2 = atomic.Load(&X)

// afterwards
if r1 == 0 && r2 == 0 {
panic("broken")
}

does the code below "afterwards" belong to a third goroutine or to
goroutine 2?

[Dmitry Vyukov]

It runs when both goroutines have stored results to r1/r2 in any
goroutine (it's not important).

[Nico]

OK, so both atomic loads happen before the execution of the code below
"afterwards".

Why is it important not to break this litmus test?

---

My opinion about issue 5045 is that:

- sync/atomic should already guarantee the litmus test 1 (I don't see a
reason not to make the change in the documentation now; I imagine
everyone using sync/atomic expects this guarantee to hold)

- the documentation in sync/atomic can be updated again once a decision
has been made about the litmus test 2.

[honzajde]

Back to the primary question: Is it safe to read condition if I add a break to the loop like this?

var condition bool

var wg sync.WaitGroup

for _, item := range items {

        if true {

          break  

        }

wg.Add(1)

go func(item) {

if meetsCondition(item) {

condition = true

}

wg.Done()

}(item)

}

wg.Wait()

// is it safe to check condition here?

There is not even one mention of WaitGroup in the spec and it's documentation is saying  Wait: "Wait blocks until the WaitGroup counter is zero." which does 

not set any happens-before relationship (or does it?). Does it mean that the first answer "It is safe to check condition after wg.Wait returns" is unofficial? If it's official what are the reasons for it I am missing? Thanks alot if you answer.

Dne středa 7. května 2014 7:45:43 UTC+2 Dmitry Vyukov napsal(a):

[Ian Lance Taylor]

On Sat, Aug 26, 2017 at 4:34 AM, honzajde <jan....@gmail.com> wrote:
>
> Back to the primary question: Is it safe to read condition if I add a break
> to the loop like this?

Yes. sync.WaitGroup.Wait would be useless if it didn't provide a
happens-before relationship from changes made in the goroutine that
calls Done to the goroutine that calls Wait.

I suppose this could be documented in the docs for sync.WaitGroup, but
it also kind of falls under the "Go is not trying to trick you" rule.

Ian

[honzajde]

I am trying to write a simple code that calls bunch of get requests in goroutines and stores result on the struct, that's all -- now I know that it is correct with just wg.Wait, ugh.... go would really suck compared to javascript if it was not true...

so far not go but godoc manages to trick me:) 

I have also discovered https://github.com/golang/go/issues/5045 - highly recommended for everyone who get's to this old post

Dne pondělí 28. srpna 2017 7:49:51 UTC+2 Ian Lance Taylor napsal(a):