What would the appropriate design pattern to avoid deadlock when several functions use the same mutex ?
It is quite easy to forget what method uses the lock and so it happens that you call a function that do a mutex.Lock() inside a function that already locked the mutex –> deadlock.
For example in the following code, it is not explicit right away that the setToNextEvenNb() is a deadlock. You have to look in nested functions. And it would be even worse if the Lock was in function called 2 or 3 level behind the setToNextEvenNb().
package main
import (
"fmt"
"sync"
)
type data struct {
value int
mutex sync.RWMutex
}
func (d *data) isEven() bool {
d.mutex.RLock()
defer d.mutex.RUnlock()
return d.value%2 == 0
}
func (d *data) setToNextEvenNb() {
d.mutex.Lock()
defer d.mutex.Unlock()
if d.isEven() {
d.value += 2
}
d.value += 1
}
func (d *data) set(value int) {
d.mutex.Lock()
defer d.mutex.Unlock()
d.value = value
}
func main() {
var d data
d.set(10)
fmt.Println("data", d.value)
fmt.Println("is even ?", d.isEven())
d.setToNextEvenNb()
fmt.Println("data", d.value)
}
When you have a large code base, this can happen quite easily.
I feel that there should be a design pattern to avoid this sort of scenario and I was looking for any advice on this ?
1
There is no particularly good way, and Golang eschews abstractions that would make risky operations safer.
Strategies you can consider:
-
Prefer very fine-grained locks. Your code mostly does this, but then has a method on your data that calls another method – a risky pattern.
As a specific variant of this, consider lock-free programming. Your example code could also be implemented with atomic operations by using compare-and-exchange style operations.
However, fine-grained locks (and especially lock-free programming techniques) are difficult to get right.
-
Use very coarse-grained locks, and distinguish between “locked” and “unlocked” operations. The main code lives in a layer where locks have already been dealt with.
Here, I think this is more appropriate, because locking within the
isEven()method is meaningless. The return value might already be outdated by the time we receive it. Instead, the code should be adapted so thatisEven()can only be called if a lock has already been acquired.However, coarser locks might lock for a longer time, potentially reducing throughput. And this is still not foolproof, especially if an operation needs to acquire multiple locks. Locking on the “outside” can be a good strategy if there are clear patterns in your code like “requests”, “transactions”, or “units of work”.
-
(In other languages:) Use a “re-entrant” or “recursive” lock, which allows the same thread to acquire the same lock multiple times. However, this cannot work with Go, as goroutines do not have an identity.
As an example of the second strategy, we might define a public interface for synchronized access to our data:
type SynchronizedData struct {
data data
mutex sync.RWMutex
}
type ReadData interface {
IsEven() bool
Get() int
}
type LockedData interface {
ReadData
Set(value int)
SetToNextEvenNb()
}
func (s *SynchronizedData) WithReadLock(op func (ReadData)) {
s.mutex.RLock()
defer s.mutex.RUnlock()
op(&s.data)
}
func (s *SynchronizedData) WithLock(op func (LockedData)) {
s.mutex.Lock()
defer s.mutex.Unlock()
op(&s.data)
}
By using a callback, the consumer of these types is informed about the scope in which the received data should be valid, though Golang cannot prevent the object from being leaked. The use of public interfaces helps achieve encapsulation of the unsynchronized data, letting a WithReadLock() user only access non-mutating methods.
We can now implement the internal data type. These operations are only accessed in a context in which the locks are already acquired, so the methods can safely call each other:
// private type
type data struct {
value int
}
func (d *data) IsEven() bool {
return d.value % 2 == 0
}
func (d *data) Get() int {
return d.value
}
func (d *data) Set(value int) {
d.value = value
}
func (d *data) SetToNextEvenNb() {
if d.IsEven() {
d.value += 2
} else {
d.value += 1
}
}
Then:
var data SynchronizedData
data.WithLock(func (d LockedData) {
d.Set(10)
})
data.WithReadLock(func (d ReadData) {
fmt.Println("data", d.Get())
fmt.Println("is even ?", d.IsEven())
})
data.WithLock(func (d LockedData) {
d.SetToNextEvenNb()
fmt.Println("data", d.Get())
})
This code is way more verbose and less convenient, but clearly shows which operations are part of which critical section, and which therefore “see” the same version of the state.
I’d like to point out that this “locking on the outside” pattern is much better supported in other languages. For example, in Rust I’d use the existing RwLock<T> type that brokers access to some inner data T:
// define "Data" within a module to make "value" private
use data::*;
mod data {
use std::sync::RwLock;
pub type SynchronizedData = RwLock<Data>;
#[derive(Debug, Clone, Default)]
pub struct Data {
value: u32,
}
// no extra interfaces/traits needed
// because Rust can already distinguish
// "mut" vs non-mutating operations
impl Data {
pub fn get(&self) -> u32 {
self.value
}
pub fn is_even(&self) -> bool {
self.value % 2 == 0
}
pub fn set(&mut self, value: u32) {
self.value = value
}
pub fn set_to_next_even_number(&mut self) {
if self.is_even() {
self.value += 2;
} else {
self.value += 1;
}
}
}
}
pub fn main() {
let data = SynchronizedData::default();
// unwrap() needed because acquiring a lock can fail
data.write().unwrap().set(10);
{
// d is a proxy object that will unlock once dropped
let d = data.read().unwrap();
println!("data={} is_even={}", d.get(), d.is_even());
// dropped here
}
{
// because we requested write access and declared "mut d",
// this section has access to all Data methods.
let mut d = data.write().unwrap();
d.set_to_next_even_number();
println!("data {}", d.get());
}
}
Check if you have recursive mutexes, that is the same mutex can be locked multiple times on the same thread. Other than this, there is a simple but slightly painful method: create a data structure containing a mutex and a level. You are not allowed any locks while a level 0 mutex is locked. You are only allowed to lock level 0 mutexes when a level 1 mutex is locked. In general, you are only allowed to lock mutexes at lower levels. And of course you need code to enforce this.
Now any potential deadlock becomes a rule violation that is detected. So you can make your code free of potential deadlocks.