remoteconfig: fine grained locking#2458
Conversation
e909608 to
53e5d7e
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Could you please add a description / motivation for this PR? Thanks! |
BenchmarksBenchmark execution time: 2024-01-08 13:18:01 Comparing candidate commit 625415c in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 39 metrics, 2 unstable metrics. |
| _callbacksMu sync.RWMutex | ||
| products map[string]struct{} | ||
| _productsMu sync.RWMutex | ||
| productsWithCallbacks map[string]ProductCallback | ||
| capabilities map[Capability]struct{} | ||
| _capabilitiesMu sync.RWMutex |
There was a problem hiding this comment.
nit: is there any reason for prefixing these fields with underscore?
There was a problem hiding this comment.
This is a personal habit, I can remove the prefixes
There was a problem hiding this comment.
It's fine. Let me review it again.
There was a problem hiding this comment.
Hm, I think this sets a bad precedence. Using underscore prefixes for go variable names, especially on structs, is highly unorthodox. It'd be nice if we could not do this going forward.
…figuration update (#2458) A deadlock occurred when trying to modify the remote configuration client using its public API while a configuration update was happening. This was due to the global lock performed in the update goroutine. This change introduces several new RWLocks for various data structures in order to avoid the global lock, and allow updating data in the client while the client is updating its configurations.
4 participants
What does this PR do?
JIRA: APPSEC-18154
This adds a finer brand of locking to the remoteconfig client in order to allow using the exported API asynchronously.
Add RW mutex for products
Add RW mutex for capabilities
Add RW mutex for callbacks
Add RW mutex for productsWithCallback
Add async testing of public API
Motivation
The current implementation relies on a global lock in the entry point of the config update routine which means that
the client can't be used when it's receiving an update. This is a problem since callbacks are called during the update
routine, and those callbacks in turn may very well need to register new products/capabilities/callbacks. This leads to a possibility of deadlock.
Adding finer locking (1 mutex/data structure, basically) allows to modify the client's state during a configurations update and in turns makes it possible for callbacks to register capabilities, products, other callbacks, etc...
Reviewer's Checklist
For Datadog employees:
@DataDog/security-design-and-guidance.Unsure? Have a question? Request a review!