Hi!
I write a lot of webserver-code, where often some central state is accessed by multiple go-routines (from concurrent web-requests). I usually safeguard access to shared state by using an sync.RWMutex like this:
var services = map[string]Service
var serviceMutex = sync.RWMutex{}
func UpdateValue(name, value Service) {
serviceMutex.Lock()
defer serviceMutex.Unlock()
services[name] = value
}
But I find this has several downsides:
One could access the variable without locking the mutex (simple forget it)
One could write a long running method, accidentally locking the variable the whole time (because defer is only called after the whole method returns)
If the access is hidden behind Get/Set Methods one could accidentally write something like v.Set(v.Get()+1) and introduce a race condition.
One could accidentally pass a reference to the variable via return (allowing unlocked access).
So I have written a small library to wrap the access to such variables and was thinking if something like this should be part of the standard library (since it seems universally useful)
I have posted the code for review and discussion on codereview.stackexchange:
These are valid points about the drawbacks of mutexes, and it is always good to see that someone is not only aware of these drawbacks but actively searches for a solution.
However, to me, the solution you propose seems a bit convoluted.
I have to define closures and pass them as arguments to Read or Update.
The Read operation relies on a side effect to get the data out of the protected data structure:
var path string
// possibly quite some lines of code between these lines
sv.Read(func(v config) { path = v.path })
As a result, the data flow may become obscured. I would prefer a straightforward syntax like path = sv.Read().
More importantly, the solution does not seem to actively prevent problems #1, #2, and #3. I played the devil’s advocate with your playground code and found the following:
I am able to create a pointer to the protected data and access it outside a mutex.
I am able to call long-running functions inside a Read or Update function.
I am able to call Read inside Update and cause a deadlock.
Here is my modified version. Search for // *** comments to find the modifications I made.
I think the problems you point out are inherent to the way concurrent access to shared memory works. Avoiding these problems either requires programming discipline or replacing concurrent access to shared memory by communication (that is, using channels and goroutine-local data).
The most correct solution if you must maintain a shared state, which I would argue can always be done in a better non-shared manner, is to use a concurrent cache that handles all the access controls for you and all your code has to do is normal map like operations and not be worried about shared state. A simple Google search turns up quite a few to choose from that are open source.
Hi @christophberger
thank you for the feedback. I want to address your points.
Closures: I was thinking closures are preferrable to defining your own functions for small actions which are only used inside a single bigger function. So you can have the benefits of a function (early returns, defer-handling on panic), but without thinking of a function name and moving the code somwhere else.
As a best practice you should usually define variables as close to usage as possible and in the narrowest scope you can. If you want to get a copy of the protected value you can just use x := sv.getCopy() but if you don’t want to copy the whole data type (might be a big map or struct) you can use a closure to extract only a single data-point
You are absolutely right I cannot stop wrong usage of the package - but I thought it is harder to accidentally get it wrong with this syntax. If you want to extract a pointer to the shared variable you have to move it out of the closure explicitly. It is clearly visible in the source code by indentation where the variable is locked - so calling a long running operation inside the Read-block should be more obvious and thus easier avoided.
Thank you for your input! I’m trying to build APIs which encourage correct usage and are harder to use wrong.
Hi @jarrodhroberson
I’m interested how you would implement a shared state like a map with configuration-options in a non-shared manner. There are Rest-endpoints which can change the configuration and endpoints which need information by the configuration to operate. So a number of these readers/writers might run concurrently, but they may also run after some time. Would you create a central go-routine with a shared channel, which is “asked” by any call to deliver the configuration over another channel? I think this could be a lot of code for little benefit compared to a shared variable?
Thank you for the input on concurrent caches. I tried searching for implementations for quire some while. But all I found where either focused on cache with eviction (not a permanent shared state) or were quite overengineered (lots of code & dependencies, for something which can be solved save in less than 100LOC) and would usually only work on maps and not e.g. on structs or other custom data-types which might be used as shared state.
Can you point me to a simple straightforward implementation I might have overlooked? I’m trying to learn and improve by looking at professional solutions.