feat(shared-runtime)!: SharedRuntime Borrowed & Owned mode#2061
Open
Aaalibaba42 wants to merge 2 commits into
Open
feat(shared-runtime)!: SharedRuntime Borrowed & Owned mode#2061Aaalibaba42 wants to merge 2 commits into
Aaalibaba42 wants to merge 2 commits into
Conversation
Aaalibaba42
force-pushed
the
jwiriath/shared-runtime-mods
branch
from
b0f4da8 to
50536ba
Compare
Contributor
📚 Documentation Check Results📦
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13d5a971de
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Contributor
Clippy Allow Annotation ReportComparing clippy allow annotations between branches:
Summary by Rule
Annotation Counts by File
Annotation Stats by Crate
About This ReportThis report tracks Clippy allow annotations for specific rules, showing how they've changed in this PR. Decreasing the number of these annotations generally improves code quality. |
Contributor
🔒 Cargo Deny Results📦
|
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: ccae7a5 | Docs | Datadog PR Page | Give us feedback! |
Aaalibaba42
force-pushed
the
jwiriath/shared-runtime-mods
branch
2 times, most recently
from
64603e7 to
4e49301
Compare
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e49301fa3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2061 +/- ##
==========================================
+ Coverage 73.47% 73.57% +0.09%
==========================================
Files 475 477 +2
Lines 78999 79219 +220
==========================================
+ Hits 58048 58283 +235
+ Misses 20951 20936 -15
🚀 New features to boost your workflow:
|
Contributor
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
|
Contributor
|
It looks like the description is a bit off mentioning fixes and bugs that seem to be coming from previous commits of the branch and not from the current state of the feature |
Contributor
Author
I tried letting a cheap AI read the commits and make a description, I don't think it's too valuable and can even be detrimental, as it were. |
Contributor
Author
|
I rewrote it cleaner and straighter to the point |
yannham
reviewed
Jun 1, 2026
Contributor
There was a problem hiding this comment.
The code LGTM overall, but I'm not entirely sure to fully grasp in which context (sync or async) is this API supposed to be consumed. In particular we use standard sync thread mechanisms (condvars, mutexes) in an async context, which could be problematic (even in a multi-thread context, a condvar will block the current executor thread, which is not ideal).
On a different front, I remember @paullegranddc said he would rather spawn our own shared runtime in a separate thread than hooking in the client's runtime. I don't have a strong opinion myself, but I wonder if this discussion had a conclusion.
Aaalibaba42
force-pushed
the
jwiriath/shared-runtime-mods
branch
from
df92857 to
9d305e7
Compare
Aaalibaba42
changed the title
Aaalibaba42
force-pushed
the
jwiriath/shared-runtime-mods
branch
from
9d305e7 to
6b5b4db
Compare
Aaalibaba42
commented
Jun 5, 2026
Aaalibaba42
force-pushed
the
jwiriath/shared-runtime-mods
branch
from
6b5b4db to
5b21367
Compare
Contributor
Author
|
Since the implementation was overhauled, I'll just resolve past conversations here, as they don't fit the current implementation. |
Aaalibaba42
force-pushed
the
jwiriath/shared-runtime-mods
branch
from
5b21367 to
61fbe96
Compare
yannham
approved these changes
Jun 8, 2026
yannham
left a comment
yannham
left a comment
Contributor
There was a problem hiding this comment.
Left a few remarks but otherwise looks good (only skimmed through the native implementation, as I suppose it's just the original shared runtime moved)👍
| #[derive(Debug)] | ||
| pub struct BorrowedSharedRuntime { | ||
| runtime: Arc<tokio::runtime::Runtime>, | ||
| workers: Arc<Mutex<Vec<WorkerEntry>>>, |
Contributor
There was a problem hiding this comment.
Should we use a blocking mutex here instead of an async one? Since we use the mutex from both sync (spawn_worker)and async (shutdown_async), it looks like an async mutex is more appropriate, as it won't block the executor thread and still offer a lock_blocking method.
Contributor
Author
There was a problem hiding this comment.
As discussed with @VianneyRuhlmann, the tokio doxa trends more towards prefering std::sync::Mutex as long as you don't hold a Mutex accross await points.
- In
spawn_workerit is locked, we push aWorkerEntry, and we drop -> no await - In
shutdown_asyncwe lock,mem::takethe vector, then dropped. The async happen on the moved-out vector after the guard is dropped.
| }) | ||
| } | ||
|
|
||
| fn runtime_handle(&self) -> Result<tokio::runtime::Handle, SharedRuntimeError> { |
Contributor
There was a problem hiding this comment.
I think we can do most operations on a handle by reference; why do we clone it here?
Contributor
Author
There was a problem hiding this comment.
A tokio::runtime::Handle is already an Arc under the hood, and giving ownership is more ergonomic than managing references ig
| /// Shutdown timed out. | ||
| ShutdownTimedOut(std::time::Duration), | ||
| /// The requested operation is not supported on a borrowed-handle runtime. | ||
| UnsupportedInBorrowedMode, |
Contributor
There was a problem hiding this comment.
Maybe this question will be answered later, but which operations return this error? Since you've split the fork support from the base, I would expect that the SharedRuntime trait API is fully supported by the borrowed runtime.
Contributor
Author
There was a problem hiding this comment.
For instance if you spawn a worker with restart_on_fork. We could separate the API in separate functions, this would make this error variant not needed, but it would also be more uneasy imho, this error is explicit enough and would trigger immediately if the shared runtime is used incorrectly, so I don't see it as a problem but I might be wrong.
Aaalibaba42
force-pushed
the
jwiriath/shared-runtime-mods
branch
from
ff3c135 to
eb913a1
Compare
Aaalibaba42
force-pushed
the
jwiriath/shared-runtime-mods
branch
from
eb913a1 to
ccae7a5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
Split
SharedRuntimeinto a trait with two implementations:OwnedSharedRuntime: owns a tokio runtime, supports fork hooks andsynchronous shutdown. Now also supports
OwnedKind::CurrentThread(spawns adedicated driver thread).
BorrowedSharedRuntime: wraps a caller-ownedArc<tokio::runtime::Runtime>,no fork hooks, no synchronous shutdown.
Why?
Some callers already have a tokio runtime they want to reuse instead of letting
libdatadog create its own. The previous
SharedRuntimeonly supported the ownedcase. Splitting into owned vs. borrowed makes the two lifecycle models explicit
and lets callers pick the one that matches their environment.
How?
SharedRuntimetrait (spawn_worker,runtime_handle,shutdown_async).OwnedSharedRuntime; addOwnedKind::{MultiThread, CurrentThread}with a driver thread for thecurrent-thread flavor.
BorrowedSharedRuntime::from_runtime(Arc<Runtime>).Rejects
restart_on_fork = truewith a newSharedRuntimeError::UnsupportedInBorrowedMode.OwnedSharedRuntime.Additional Notes
Breaking change for Rust callers of
libdd_shared_runtime:SharedRuntimeisnow a trait; use
OwnedSharedRuntime::new()instead ofSharedRuntime::new().FFI handle type changes from
SharedRuntimetoOwnedSharedRuntime.