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: document more clearly sync.RWMutex doesn't support lock upgrades/downgrades #38859

Open
sfllaw opened this issue May 4, 2020 · 6 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Documentation NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@sfllaw
Copy link
Contributor

sfllaw commented May 4, 2020

What version of Go are you using (go version)?

$ go version
go version go1.14.2 linux/amd64

What did you do?

Tried to figure out the correct semantics for upgrading and downgrading read and write locks in sync.RWMutex.

What did you expect to see?

Unlike sync.Mutex, working with sync.RWMutex is a bit tricky especially when it comes to upgrading and downgrading read and write locks. #4026 documents how one should perform these operations, but this isn’t linked in the documentation itself.

I propose an example that demonstrates upgrading and downgrading locks. Maybe something like this?

package sync_test

import (
	"fmt"
	"sync"
)

type Map struct {
	mu   sync.RWMutex
	data map[interface{}]interface{}
}

func (m *Map) Load(key interface{}) (value interface{}, ok bool) {
	m.mu.RLock()
	defer m.mu.RUnlock()

	v, ok := m.data[key]
	return v, ok
}

func (m *Map) LoadOrStore(key, value interface{}) (actual interface{}, loaded bool) {
	m.mu.RLock()
	defer m.mu.RUnlock()

	loaded = true

	// Retry until actual can be read from m.data so the first LoadOrStore wins:
	for ok := false; !ok; {
		if actual, ok = m.data[key]; ok {
			break
		}

		loaded = false

		// Upgrade the read lock to a write lock:
		m.mu.RUnlock()
		m.mu.Lock()

		if m.data == nil {
			m.data = make(map[interface{}]interface{})
		}
		m.data[key] = value

		// Downgrade the write lock to a read lock:
		m.mu.Unlock()
		m.mu.RLock()
	}

	return actual, loaded
}

func ExampleRWMutex() {
	c := Map{}

	var wg sync.WaitGroup
	wg.Add(1)

	go func() {
		for {
			if v, ok := c.Load("greet"); ok {
				fmt.Println("Load:", v)
				break
			}
		}
		wg.Done()
	}()

	v, loaded := c.LoadOrStore("greet", "hello")
	fmt.Println("LoadOrStore:", v, loaded)

	wg.Wait()

	// Output:
	// LoadOrStore: hello false
	// Load: hello
}
@sfllaw sfllaw changed the title Add examples for upgrading and downgrading sync.RWMutex Add examples for upgrading and downgrading locks in sync.RWMutex May 4, 2020
@ianlancetaylor
Copy link
Contributor

Thanks, but I'm not in favor of this, because I would say that RWMutex does not support upgrading or downgrading locks. As your example shows, the only mechanism is to unlock the mutex and then lock it again. But the normal definition of upgrading a reader lock to a writer lock is that upgrading the lock is an atomic operation: if you upgrade the lock, no other goroutine can acquire the writer lock. Unlocking an RWMutex will permit another goroutine to grab the writer lock between the RUnlock and the Lock, so it is not what I would describe as an upgrade operation.

@ianlancetaylor ianlancetaylor added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label May 4, 2020
@sfllaw
Copy link
Contributor Author

sfllaw commented May 11, 2020

@ianlancetaylor Normally, I find the Go standard library documentation to be very instructive as to the correct way to use it, so I was surprised that RWMutex did not have good documentation about upgrading and downgrading locks.

It is true that there is no atomic upgrade or downgrade, but this is not mentioned at all. At the very least, I think it would be useful to mention this in the doc comment. My request for an example is to provide guidance on how to use RWMutex in spite of the fact that it isn't atomic.

In fact, there is a subtle bug in my example where two LoadOrStore calls might race to return their own value arguments, showing how tricky this mutex is to use:

func (m *Map) LoadOrStore(key, value interface{}) (actual interface{}, loaded bool) {
	m.mu.RLock()
	defer m.mu.RUnlock()

	loaded = true

	// Retry until actual can be read from m.data so the first LoadOrStore wins:
	for ok := false; !ok; {
		if actual, ok = m.data[key]; ok {
			break
		}

		// Upgrade the read lock to a write lock:
		m.mu.RUnlock()
		m.mu.Lock()

		if m.data == nil {
			m.data = make(map[interface{}]interface{})
		}

		// Upgrading the lock is not atomic, so another writer may have raced us:
		if _, ok := m.data[key]; !ok {
			m.data[key] = value		
			loaded = false
		}

		// Downgrade the write lock to a read lock:
		m.mu.Unlock()
		m.mu.RLock()
	}

	return actual, loaded
}

@davecheney
Copy link
Contributor

Have you benchmarked this? Is it faster than a single map plus mutex or striped mutex?

@sfllaw
Copy link
Contributor Author

sfllaw commented May 14, 2020

@davecheney: Sorry if I wasn’t clear: the exact example does not matter. We can substitute any example that protects something with many readers and a single writer.

This feature request is to add an example for how to safely upgrade and downgrade a write lock on sync.RWMutex because there is no atomic way to do so. The key to this example is to explicitly show the need to re-read before and after the write.

@tliron
Copy link

tliron commented May 21, 2021

I suggest simply stating in the RWMutex documentation that it does not support upgrading or downgrading locks. The text right now seems to be trying to say that, but it's honestly hard to follow:

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.

How about this instead:

If a goroutine holds a RWMutex with either RLock or Lock then it cannot acquire either lock again without first releasing the current hold. This prohibits recursive locking as well as upgrading an RLock to a Lock or downgrading a Lock to an RLock.

We can then provide an example that shows that because upgrades are impossible it is important to take into account that other goroutines may acquire a lock between a call to RUnlock and Lock. It basically means that if you're trying to do a CAS-style atomic operation then you must repeat the comparision after the Lock, because the state may very well have changed between the RUnlock and the Lock.

@seankhliao seankhliao changed the title Add examples for upgrading and downgrading locks in sync.RWMutex sync: add examples for upgrading and downgrading locks in sync.RWMutex May 21, 2021
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 7, 2022
@seankhliao seankhliao added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Aug 27, 2022
@seankhliao seankhliao added this to the Unplanned milestone Aug 27, 2022
@seankhliao seankhliao changed the title sync: add examples for upgrading and downgrading locks in sync.RWMutex sync: document more clearly sync.RWMutex doesn't support lock upgrades/downgrades Aug 27, 2022
@cgthayer
Copy link

I agree 100% that this should be made clearer in the documentation. It's still not clear enough.
As a systems programmer my expectation is that the locking for RW locks is via a queue so that a waiting writer (or reader) deterministically can't be starved forever, but given this implementation it sounds like this is not guaranteed. Is this true?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Documentation NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
Status: Triage Backlog
Development

No branches or pull requests

7 participants