use sync.RWMutex when masterNode changes #75

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@yanyiwu
Contributor
yanyiwu commented Feb 1, 2015

#74

@yanyiwu yanyiwu referenced this pull request Feb 1, 2015
Closed

single point of failure #74

@chrislusf
Owner

No need for this. Only one goroutine will be changing this value.

@chrislusf chrislusf closed this Feb 1, 2015
@yanyiwu
Contributor
yanyiwu commented Feb 1, 2015

But other goroutine will read this value.

@yanyiwu
Contributor
yanyiwu commented Feb 1, 2015

I wrote two programs to check if safe.

https://github.com/aszxqw/practice/blob/master/go/rwmutex/notlock.go
https://github.com/aszxqw/practice/blob/master/go/rwmutex/lock.go

It turns out go run notlock.go will go wrong(get stuck) after running 2 minutes.
And go run lock.go is always running ok .

So I think it is not safe, when one goroutine is writing a variable and other goroutines read it without locking.
So I think you need this pull request.

@chrislusf
Owner

I do not understand the test. Does it mean g.a will be read wrongly? Wrongly means totally wrong data or just stale data? Stale data is fine for current bug, since the thread to check correct master node only runs every few seconds.

@yanyiwu
Contributor
yanyiwu commented Feb 1, 2015

Not about stale data, it is about something unexpected will happen when concurrent access without sync.
If it is safe for one goroutine writing when another goroutine reading without sync,
so what do you think of sync.RWMutex is made for ?

@yanyiwu
Contributor
yanyiwu commented Feb 1, 2015

"Programs that modify data being simultaneously accessed by multiple goroutines must serialize such access.
To serialize access, protect the data with channel operations or other synchronization primitives such as those in the sync and sync/atomic packages."

https://golang.org/ref/mem

@chrislusf
Owner

We have one thread that's basically swapping one string value to another. This is a copy on write operation. Maybe check this page, which explains better than I do: http://en.wikipedia.org/wiki/Copy-on-write

@yanyiwu
Contributor
yanyiwu commented Feb 3, 2015

I know the conception of COW, but I don't know what is the relation between COW and this concurrency-safety issue.

I guess your thoughts is "it is safe because masterNode is a string ", isn't it ?
So I wanna ask you a question , is it safe when masterNode is a int or map ?

@jan4984
jan4984 commented Dec 11, 2015

i got "Failed to write to replicas for volume 3" too and it leads me here. not know much to the seaweedfs source but about the concurrency-safety, I think @yanyiwu is right, we need lock.

@chrislusf
Owner

Please share your logs, setups.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment