fix(shared-runtime): make ForkSafeRuntime teardown structurally TLS/poison-safe#2220
Draft
quinna-h wants to merge 5 commits into
Draft
fix(shared-runtime): make ForkSafeRuntime teardown structurally TLS/poison-safe#2220quinna-h wants to merge 5 commits into
quinna-h wants to merge 5 commits into
Conversation
…uring CPython finalization During CPython interpreter finalization, thread-local storage is destroyed before atexit handlers fire. SharedRuntime::shutdown() calls runtime.block_on() which internally calls context::enter() to set up Tokio's CONTEXT thread-local. If that TLS slot is already destroyed, context::enter() panics with "The Tokio context thread-local variable has been destroyed", which PyO3 converts to a pyo3_runtime.PanicException. This causes a crash on every uWSGI worker shutdown when using ddtrace >=4.9.x. Fix: check Handle::try_current().is_thread_local_destroyed() before calling block_on(). If TLS is gone, return Ok(()) early — the OS will clean up remaining Tokio threads on process exit. This eliminates both the panic and the subsequent 60s hang/SIGKILL. Reproducer: uWSGI app with lazy-apps=true, ddtrace imported via uwsgi import=, 4 workers. SIGTERM triggers the panic on every worker. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… refactor Resolves merge conflict with main, which refactored SharedRuntime into separate ForkSafeRuntime, BasicRuntime, and LocalRuntime types. The TLS destruction guard (Handle::try_current().is_thread_local_destroyed()) is now applied to ForkSafeRuntime::shutdown() in fork_safe.rs, where the block_on call lives after the refactor. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…oison-safe Replaces the symptomatic shutdown()-only TLS guard with two structural invariants so teardown does not depend on the destruction order of any other global/thread-local state during (embedded) interpreter finalization: A. Terminal teardown never blocks or enters the Tokio context. Add `Drop for ForkSafeRuntime` that detaches the owned runtime via `shutdown_background()` instead of a normal blocking `Runtime` drop (which joins worker threads and touches the CONTEXT thread-local, panicking with "The Tokio context thread-local variable has been destroyed" when that TLS is already gone). Done unconditionally, so there is no finalization-detection branch a future teardown scenario can slip past. The graceful `shutdown()` path (which flushes) still runs when a live context exists (`can_block_on`); if it already ran, Drop is a no-op. B. Shared-state access is poison-immune. Add `MutexExt::lock_or_recover` (recovers the guard via `into_inner()` instead of unwrapping) and use it on all runtime/workers lifecycle locks, so one thread panicking cannot cascade into a `PoisonError` on every other thread that later takes the lock. Adds tests: drop-without-shutdown detaches cleanly, and shutdown recovers from a poisoned lock. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
📚 Documentation Check Results📦
|
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📦
|
Contributor
|
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
|
VianneyRuhlmann
requested changes
Jul 10, 2026
|
|
||
| #[inline(always)] | ||
| fn lock_or_recover(&self) -> MutexGuard<'_, T> { | ||
| self.lock().unwrap_or_else(|poisoned| poisoned.into_inner()) |
Contributor
There was a problem hiding this comment.
The PoisonError should not be used, we have no guarantee over the state in which the value was left.
| fn drop(&mut self) { | ||
| if let Some(runtime) = self.runtime.lock_or_recover().take() { | ||
| match Arc::try_unwrap(runtime) { | ||
| Ok(runtime) => runtime.shutdown_background(), |
Contributor
There was a problem hiding this comment.
This won't trigger the shutdown logic of the workers and therefore will not flush data before the fork this will cause the same issue as --skip-atexit
…estroyed shutdown() previously skipped the graceful shutdown entirely once the calling thread's Tokio CONTEXT thread-local was destroyed (embedded interpreter finalization), deferring to Drop's shutdown_background(), which detaches without flushing in-flight work. Since only the calling thread's TLS is affected, not the runtime's own worker threads, shutdown() now hands the same graceful shutdown off to a freshly spawned thread whose TLS was never touched, and waits for it via a plain channel. This lets a final flush actually happen during finalization instead of being silently dropped.
BenchmarksComparisonBenchmark execution time: 2026-07-10 16:46:11 Comparing candidate commit 302c31d in PR branch Found 18 performance improvements and 41 performance regressions! Performance is the same for 212 metrics, 10 unstable metrics.
|
Contributor
|
I fundamentally worry we're going the wrong direction here. Tokio has undocumented global state, and may just be incompatable with forking. We can play whack-a-mole with fork-based crashes on a multi-threaded runtime, but do we have a design where we believe we ought to eventually converge on true fork safety? |
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 does this PR do?
Replaces the
shutdown()-only TLS guard with two structural invariants soForkSafeRuntimeteardown never depends on the destruction order of other global/thread-local state during (embedded) interpreter finalization.Motivation
During (embedded) interpreter finalization, a
ForkSafeRuntimecan be torn down after the calling thread's Tokio CONTEXT thread-local has already been destroyed. When that happens, teardown panics with"The Tokio context thread-local variable has been destroyed":shutdown()callsruntime.block_on(...), which enters the Tokio context via the thread-local — panicking if it's already gone.shutdown(), a normalRuntimedrop blocks the current thread to join its worker threads, and that join path also touches the context thread-local, so a worker thread panics during drop.Because
lock_or_panic()unwraps a poisoned mutex, that first panic then cascades: every other thread that later locks the shared runtime/workers mutex panics too withPoisonError.This surfaces as a crash on every uWSGI worker shutdown with ddtrace ≥4.9.x (lazy-apps +
--die-on-term). It's fundamentally an ordering problem; the process-global runtime, its worker threads, and thread-local state are destroyedin an order the library doesn't control, and under uWSGI the teardown is driven from the C layer with no reliable point to shut down cleanly beforehand (the worker's SIGTERM is handled by uWSGI; Python's atexit runs only after TLS is
already destroyed). So the terminal teardown path must be safe unconditionally, rather than relying on detecting the finalization state at each call site.
Additional Notes
Anything else we should know when reviewing?
How to test the change?
Describe here in detail how the change can be validated.