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

Add examples for upgrading and downgrading locks in sync.RWMutex #38859

Open
sfllaw opened this issue May 4, 2020 · 4 comments
Open

Add examples for upgrading and downgrading locks in sync.RWMutex #38859

sfllaw opened this issue May 4, 2020 · 4 comments

Comments

@sfllaw
Copy link

@sfllaw 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

@ianlancetaylor ianlancetaylor commented May 4, 2020

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.

@sfllaw
Copy link
Author

@sfllaw 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

@davecheney davecheney commented May 11, 2020

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

@sfllaw
Copy link
Author

@sfllaw 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 download 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.