NebulousLabs / Sia

Blockchain-based marketplace for file storage. Project has moved to GitLab: https://gitlab.com/NebulousLabs/Sia
https://sia.tech
MIT License
2.71k stars 442 forks source link

Remove unnecessary lock #3085

Closed LeeSmet closed 6 years ago

LeeSmet commented 6 years ago

In the Peers() function, the read lock is already acquired, so this lock is unnecessary.

DavidVorick commented 6 years ago

That's actually a deadlock risk. Calling Rlock; Rlock puts you at risk of deadlock if someone calls a writelock in another thread between the Rlock calls.

I wonder if this has happened anywhere, though I haven't seen any user reports indicating a deadlocked gateway.

ChrisSchinnerl commented 6 years ago

@DavidVorick I don't think the other thread would be able to acquire a write lock while you hold the readlock. It should only be able to acquire the write lock after you called RUnlock twice.

lukechampine commented 6 years ago

The docstring for RWMutex says:

If a goroutine holds a RWMutex for reading and another goroutine might call Lock, no goroutine should expect to be able to acquire a read lock until the initial read lock is released. In particular, this prohibits recursive read locking. This is to ensure that the lock eventually becomes available; a blocked Lock call excludes new readers from acquiring the lock.

ChrisSchinnerl commented 6 years ago

That's interesting. Didn't know that. Makes me wonder why it's not enforced upon acquiring the lock for the second time.

DavidVorick commented 6 years ago

It can't tell if it's the same thread grabbing the lock twice. And even if it could, that's still not a guarantee because things like waitgroups can mean that multiple different threads could end up being required to grab the read locks in a specific, accidentally recursive order.