fix(library-config): update Linux process context#2237
Conversation
📚 Documentation Check Results📦
|
🔒 Cargo Deny Results📦
|
BenchmarksComparisonCandidateCandidate benchmark detailsBaselineBaseline benchmark details |
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 97138bc | Docs | Datadog PR Page | Give us feedback! |
1a04bb0 to
e6af895
Compare
e5a142d to
a9e0f0e
Compare
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
|
e6af895 to
89a636d
Compare
a9e0f0e to
97138bc
Compare
yannham
left a comment
There was a problem hiding this comment.
Thanks for stress-testing and fixing the fork case! Mostly non-blocking comments. If I understand correctly, if MADV_DONTFORK isn't supported and a fork happens, we don't have to do anything special because update, publish or unmap would touch the copy-on-write copy of the child. The only issue being that for some small time interval the child will appear as having the same context as its parent, instead of its own (which is why we are using MADV_DONTFORK in the first place). Is that more or less correct?
| // should anything have been written already we would get a short write | ||
| // However, this is not the case for macOS, despite what its manual | ||
| // says: See https://github.com/apple-oss-distributions/xnu/blob/5c306bec31e314fa4d8bbdafb2f6f5a6b7e7b291/bsd/man/man2/write.2#L168-L186 | ||
| pipe_dirty: true, |
There was a problem hiding this comment.
Should it then be cfg-gated, instead of using true for both platforms? Or maybe we don't really care, because putting true for Linux just causes the pipe to be reinitialized which is fine?
There was a problem hiding this comment.
Yeah I'd go for one simpler approach that works for both, this code is "weird enough" as-is ;)
| @@ -63,7 +60,7 @@ impl super::HeaderMemoryHolder for MemMapping { | |||
| } | |||
|
|
|||
| fn after_fork(self) { | |||
There was a problem hiding this comment.
What's the use-case for after_fork? If it's the same as drop, and drop is called automatically anyway when we exit a scope, it looks like it's a no-op
| (*header) | ||
| .payload_size | ||
| .store(payload_size, Ordering::Relaxed); | ||
| (*header) |
There was a problem hiding this comment.
Any reason for the re-ordering here? Not that it's important, since both accesses are Relaxed, this shouldn't change anything from the point of view of the reader.
| // If we've been forked, we need to prevent the mapping from being dropped | ||
| // normally, as it would try to unmap a region that isn't mapped anymore in the | ||
| // child process, or worse, could have been remapped to something else in the | ||
| // meantime. | ||
| // | ||
| // To do so, we get the old handler back in `local_handler` and prevent `mapping` | ||
| // from being dropped specifically. | ||
| swap(&mut local_handler, handler); | ||
| local_handler.mapping.after_fork(); |
There was a problem hiding this comment.
I wonder if all the comment and the local_handler dance is relevant anymore. Now that you've pushed the check to the unmap implementation, I think we can just use swap and call it a day.
ivoanjo
left a comment
There was a problem hiding this comment.
I'd like to know more about the MADV_DONTFORK change before we apply it 👀
| // should anything have been written already we would get a short write | ||
| // However, this is not the case for macOS, despite what its manual | ||
| // says: See https://github.com/apple-oss-distributions/xnu/blob/5c306bec31e314fa4d8bbdafb2f6f5a6b7e7b291/bsd/man/man2/write.2#L168-L186 | ||
| pipe_dirty: true, |
There was a problem hiding this comment.
Yeah I'd go for one simpler approach that works for both, this code is "weird enough" as-is ;)
| /// `Some(pid)` when `MADV_DONTFORK` succeeded, otherwise `None`. | ||
| only_for_pid: Option<u32>, |
There was a problem hiding this comment.
Wait, when can MADV_DONTFORK fail on Linux? It's not clear from the comments/PR description why we're treating it as might-fail now.
In particular, the spec and other implementations are requiring MADV_DONTFORK; this is not to say that the spec is correct -- perhaps there's a good reason to change this, which is why I'm asking.
What does this PR do?
This is 2/3 in the stacked process-context series and depends on #2228.
It adapts the Linux implementation after the code-organization layer:
MADV_DONTFORKbest-effort instead of treating it as a publishing failure;Why?
#2228 intentionally preserves the old Linux behavior while introducing the shared reader/writer structure. This PR applies the Linux behavior changes separately so they can be reviewed without being mixed with either the file reorganization or the macOS/Windows implementation.
Impact
Linux keeps the same platform-agnostic process-context API, with corrected fork handling and the final feature defaults. This layer does not add macOS or Windows support.
Validation
Validated independently at this commit with:
cargo check -p libdd-library-config --all-features;cargo check -p libdd-library-config-ffi;cargo nextest run -p libdd-library-config --all-features --no-fail-fast;cargo nextest run -p libdd-library-config --no-default-features --features process-context-reader --no-fail-fast.Stack