Repository navigation
fix(storage): shut down dedicated runtime on engine drop to break self-cycle leak - #20
Open
detail-app[bot] wants to merge 2 commits into
Conversation
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
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.
Detail bug report: View on Detail
Bug
A disk cache built with a dedicated runtime via
with_spawner(Spawner::Runtime(...))(the path used by the README quickstart andexamples/hybrid_full.rs) leaks the entire dedicatedtokio::Runtime— worker threads, pending tasks, channels, and buffers — when the cache is dropped in a long-running process.Root cause: the block engine spawns its flusher/reclaimer workers as tasks on that dedicated runtime, and each worker future captures a
Spawnerclone (Arc<BackgroundShutdownRuntime>). This forms a self-cycle —Arc<BackgroundShutdownRuntime>→Runtime→ spawned task future →Spawner(Arc<…>)— that keeps theArcstrong count above zero forever. SinceRuntime::shutdown_background()was only ever called fromBackgroundShutdownRuntime::drop(which fires on the lastArcclone), it never ran: the runtime was never torn down. The defaultSpawner::Handlepath was unaffected (noArc<BSR>exists there; the external runtime owns the lifecycle).Fix
foyer-common/src/spawn.rs: RedesignedBackgroundShutdownRuntimeto holdMutex<Option<Runtime>>+ a storedHandle(spawning now goes throughDeref<Target=Handle>;Handleexposes bothspawnandspawn_blocking, which are the only methods used in-tree). Added an idempotentBackgroundShutdownRuntime::shutdown()that takes theRuntimeout of theMutexand callsshutdown_background()(ordrop(runtime)undermadsim);Dropcalls it. AddedSpawner::shutdown()(no-op forSpawner::Handle).shutdown_backgroundis non-blocking (signals + returns; worker threads self-exit), so it is safe even when called from within the dedicated runtime itself.foyer-storage/src/engine/block/engine.rs: Addedimpl Drop for BlockEngineInnerthat callsself._spawner.shutdown(), forcing the dedicated runtime to tear down when the engine is dropped. The forced shutdown drops the spawned flusher/reclaimer task futures (releasing their capturedArc<…>clones), so theArc<BackgroundShutdownRuntime>finally reaches zero.foyer-storage/Cargo.toml: Addedrt-multi-thread,macrosto thetokiodev-dependency so the dedicated-runtime regression tests (which build anew_multi_threadruntime) compile under single-packagecargo nextest run -p foyer-storage.Testing
Added 4 regression tests (gated
#[cfg(not(madsim))]) that hold only aWeak<BackgroundShutdownRuntime>acrossdrop(store)/drop(engine)and assertweak.upgrade().is_none()afterward (impossible before the fix — the bug report's repro showedweak.upgrade().is_some()=true):store::tests::test_dedicated_runtime_released_on_drop— drop afterwait()store::tests::test_dedicated_runtime_released_on_close_then_drop—close()does not tear down (still alive afterclose()), but subsequentdrop()releases the runtimestore::tests::test_dedicated_runtime_no_accumulation— droppingN=4caches releases allNruntimes (no linear growth)engine::block::engine::tests::test_dedicated_runtime_released_on_engine_drop— engine-level teardown works without theStorewrapperVerification:
foyer-common(36),foyer-storage(25 default / 28 withtest_utilsincl. fuzzy),foyer-memory(31),foyer(12 incl. hybrid fuzzy). No regressions on the defaultSpawner::Handlepath.cargo fmt --all -- --checkand nightlyffmtboth clean;cargo clippyclean on the touched crates (only the pre-existing environmentalchunks_exact_to_as_chunksunknown-lint note).#[cfg(madsim)] drop(runtime)shutdown branch is exercised viaBlockEngineInner::Dropin everytest_store_*test.cargo run -p examples --example hybrid_fullcompletes without hanging;cargo run -r -p foyer-bench -- --runtime dedicated ...completes withClose takes: 6.7ms(clean teardown instead of leaking until process exit).Automatic Fixes PRs can be configured here.