I have a type with two methods; Add & Close. The Add method is accessed concurrently and it should check if Close was ever called.
type foo struct {
closed bool
}
func (f *foo) Close() error {
f.closed = true
...
}
func (f *foo) Add(...) error {
if f.closed {
return ErrClosed
}
...
}
This is a race condition right?
So would using an atomic store/load in this situation make sense?
type foo struct {
closed int32
}
func (f *foo) Close() error {
atomic.StoreInt32(&f.closed, 1)
...
}
func (f *foo) Add(...) error {
if atomic.LoadInt32(&f.closed) == 1 {
return ErrClosed
}
...
}
Or would a channel be a more idiomatic way to do this:
type foo struct {
closed chan struct{}
}
func NewFoo() *foo {
return &foo{make(chan struct{})}
}
func (f *foo) isClosed() bool {
select {
case <-f.closed:
return true
default:
}
return false
}
func (f *foo) Close() error {
close(f.closed)
...
}
func (f *foo) Add(...) error {
if f.isClosed() {
return ErrClosed
}
...
}
EDIT:
Thank you for the comments, I ended up with this.
type foo struct {
closed bool
closeLock sync.RWMutex
}
func (f *foo) Close() error {
f.closeLock.Lock()
defer f.closeLock.Unlock()
f.closed = true
...
}
func (f *foo) Add(...) error {
f.closeLock.RLock()
defer f.closeLock.RUnlock()
if f.closed {
return ErrClosed
}
...
}
This is a race condition right?
If Close() is being called concurrently with Add(), it is indeed a data race. "Race condition" is a more general term and atomics only prevent data races, not all race conditions. The definition of concurrently is based on https://golang.org/ref/mem but if there is no coordination and it is being done from multiple goroutines, it is concurrent.
So would using an atomic store/load in this situation make sense?
Strictly speaking, it is unlikely it would make sense. What you have written is data race free. However, atomics are tricky. For example, if you now added an atomic int64 to this struct, it would have dataraces on x86-32 machines for not being 64 bit aligned while if you ordered the int64 first it wouldn't. Atomics can be dangerous and should be used with extreme caution.
Or would a channel be a more idomatic way to do this
Yes! You can also use a sync.Mutex or even sync.RWMutex. Atomics, unless you are literally writing the sync.Mutex implementation, are an optimization. You should always avoid them in a first implementation. If you are having measurable lock contention, you can then look into using atomics. Just be very careful and realize that there is a good chance of messing it up.