Protect object map from concurrent access #340
Conversation
export.go
Outdated
| @@ -396,7 +395,7 @@ func (conn *Conn) export(methods map[string]reflect.Value, path ObjectPath, ifac | |||
|
|
|||
| // If this is the first handler for this path, make a new map to hold all | |||
| // handlers for this path. | |||
| if !h.PathExists(path) { | |||
| if _, ok := h.PathExists(path); !ok { | |||
There was a problem hiding this comment.
Better get a write lock here then and keep it until the end of the function - this also removes the need for checking the other PathExists in L408.
There was a problem hiding this comment.
The main problem I can see with that is what lock should be taken, I would argue that it is not a good design to call h.Lock() from this context and if we choose to do that we need to remove the lock call in h.AddObject too, making it asymmetric to h.RemoveObject. To me it is better to let defaultHandler and its functions handle its own locks.
There was a problem hiding this comment.
But the way this is implemented right now introduces a race condition. If you have two export calls running at the same time for the same path, it could happen that:
- Call 1 reaches export.go:398. There's no object on that path yet, so
okis false. AfterGetExportedObjectreturns, the lock is not held. - Call 2 goes through the whole function, working on the newly created entry in
h.objects. - Call 1 is scheduled again and reaches export.go:399, overwriting the entry in
h.objectsand thus discarding the effect that Call 2 had.
There was a problem hiding this comment.
(sorry about the delay..)
@guelfey, Added a new function to defaultHandler, GetAnExportedObject(path). This function will return either an existing object for the path or register a newly created object on the path and return it. It should address your concerns above but I'm not sure that it is a good name though.
Note, I've yet to run it through our own use cases to make sure it works....
GetAnExportedObject returns either an existing object or a newly created object for a specific path. This will solve the case where we could get a mixup when two different goroutines competed to create an object for a specific path. The result might still be unsatifying but atleast its either or and not a mix.
| h.RLock() | ||
| defer h.RUnlock() |
There was a problem hiding this comment.
This needs to be a write lock then. The race detector picked this up as well: https://github.com/godbus/dbus/actions/runs/4797626686/jobs/9033883115?pr=340#step:6:243
GetOrAddExportedObject returns either an existing object or a newly created object for a specific path. This will solve the case where we could get a mixup when two different goroutines competed to create an object for a specific path. The result might still be unsatifying but atleast its either or and not a mix.
|
@guelfey Hi, would it be possible to move this forward or do you think there is more needed by me? |
| obj, ok := h.objects[path] | ||
| return obj, ok |
There was a problem hiding this comment.
| obj, ok := h.objects[path] | |
| return obj, ok | |
| return h.objects[path] |
In situations with many concurrent dbus requests and at the same time new objects are registered it is possible to end up in a situation where the object map is write and read accessed from different go routines resulting in a panic.
The patch is working in our setup but I've not run any test other than 'go test'