Repository navigation
fix(sidecar): avoid telemetry cache lock inversions - #2562
Conversation
Snapshot telemetry client handles before stats and flush inspect them. This keeps the cache lock out of application and client lock scopes while preserving Stop sequencing. Guard Stop removal by client identity and cover retirement, snapshot lifetime, and replacement races with regression tests.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Working through the strongest version of this alternative: after Abbreviated, I understand that implementation to look like this: if let Some(last_handle) = last_handle {
last_handle.await.ok();
}
let (processed, stopped_client) = {
let mut guard = telemetry_mutex.lock_or_panic();
let processed = guard
.as_mut()
.map(|client| client.process_actions(actions))
.unwrap_or_default();
let stopped_client = guard.take();
(processed, stopped_client)
}; // Release the client lock.
telemetry_clients.remove_if_stopped(service, env, &telemetry_mutex);
drop(stopped_client);with removal approximately: fn remove_if_stopped(&self, key: &Key, expected: &ClientHandle) {
let mut clients = self.inner.lock_or_panic();
let should_remove = clients.get(key).is_some_and(|entry| {
Arc::ptr_eq(&entry.client, expected)
&& entry.client.lock_or_panic().is_none()
});
if should_remove {
clients.remove(key);
}
}But during the interval between Stop's Swapping removal and The snapshot approach avoids changing these lifecycle semantics. Stop still removes the entry Snapshotting is not free: each stats or flush operation allocates a |
BenchmarksComparisonCandidateCandidate benchmark detailsBaselineBaseline benchmark details |
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 5d183fe | Docs | View more details | Give us feedback! |
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
|
@cataphract I've looked at it more closely and found that the nested case for taking the lock no longer applies anyway: https://github.com/DataDog/libdatadog/compare/bob/deadlock?expand=1 |
|
@bwoebi I like the direction of releasing the client lock before going for the the cache lock. I'm not sure, however, that removing snapshotting is the best direction.
Other notes:
This doesn't do much. It can still become
Finally, stepping back a bit... This dance with the two exposed locks (three if we count the application map lock involved in the stats-enqueue deadlock) is not great. I think a better one would be to have only the cache lock and have a separate actor (a tokio task) handling, in a serialized fashion, the actions for that specific client. In any case, feel free to move forward with your commit, the most important thing is to fix the deadlocks. |
Yeah, I'm aware. It's just about making the window a bit smaller. The code isn't optimal and probably should be overhauled anyway eventually. But yes, merging this now, to avoid the deadlock.
Agree, however my intuition would be to not snapshot ... until we prove that snapshotting is the right direction. |
|
/merge |
|
View all feedbacks in Devflow UI.
It will be processed automatically as soon as GitHub reports it as mergeable. View in MergeQueue UI.
The expected merge time in
PR can't be merged according to github policy |
yannham
left a comment
yannham
left a comment
There was a problem hiding this comment.
Agreed with @cataphract for the longer term, actor-based architecture. A three mutex dance is just asking for deadlocks to happen...
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: rejected
and removed
mergequeue-status: in_progress
labels
Queued tasks now own their converted actions and cloned worker handles. The existing last_handle chain still ensures that preceding batches are enqueued before Stop. Consequently, take() no longer needs to run inside the spawned task.
5a820e5 to
5d183fe
Compare
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: queued
mergequeue-status: in_progress
and removed
mergequeue-status: queued
labels
3 participants
What does this PR do?
Snapshot telemetry client handles before stats and flush inspect them. This keeps the cache lock out of application and client lock scopes while preserving Stop sequencing.
Guard Stop removal by client identity and cover retirement, snapshot lifetime, and replacement races with regression tests.
How to test the change?
See DataDog/dd-trace-php#4219