From ccef521aa73e724d25c40e60ec3c08f2b0863e3b Mon Sep 17 00:00:00 2001 From: nsfisis Date: Fri, 17 Jul 2026 22:12:35 +0900 Subject: perf(sync-executor): drive HTTP fetches through one real top-level runtime Replace sync_executor::block_on's reactor-less busy-spin poller with tokio::task::block_in_place + Handle::current().block_on(), riding a single tokio Runtime entered once in main.rs (falling back to a disposable one when no ambient runtime exists, e.g. in tests). This lets HttpDownloader::dispatch await CurlDownloader::download directly instead of bouncing through the separate curl_runtime() bridge, which is now deleted. Manual create-project verification against the real network caught a concurrency bug this exposed: async_fetch_file held http_downloader's RefMut across the await on add(), which only panics once downloads genuinely overlap. add() only needs &self, so borrow() fixes it. With everything now sharing one real reactor, the FuturesOrdered fan-out added for ComposerRepository::get_security_advisories/ load_async_packages finally overlaps for real: fetching 8 packages' metadata dropped from ~7-40s to a consistent ~3-4s in a before/after comparison, with identical resulting lock files. sync_executor::block_on's call sites are still synchronous rather than async fn propagated up to Command::execute, which remains the end goal (see the TODO(phase-e) in sync_executor.rs) - nested block_on calls elsewhere don't get this same overlap, only prevented panics. --- crates/shirabe/src/util/sync_executor.rs | 61 +++++++++++++++++++------------- 1 file changed, 37 insertions(+), 24 deletions(-) (limited to 'crates/shirabe/src/util/sync_executor.rs') diff --git a/crates/shirabe/src/util/sync_executor.rs b/crates/shirabe/src/util/sync_executor.rs index ee0995fc..8ba45bd2 100644 --- a/crates/shirabe/src/util/sync_executor.rs +++ b/crates/shirabe/src/util/sync_executor.rs @@ -1,34 +1,47 @@ -//! Minimal synchronous future executor used as a drop-in for the `tokio::runtime::Runtime::new()` -//! + `block_on` sync bridges scattered across the codebase (repository / installer / downloader). +//! Sync-to-async bridge used across the codebase (repository / installer / downloader) at the +//! many call sites that still have a synchronous signature but need to drive an `async fn`. //! -//! Those bridges drive `async fn`s whose `.await` points all resolve synchronously — this relies -//! on the invariant below. `CurlDownloader` no longer qualifies: it now performs real async I/O -//! via a non-blocking `reqwest::Client`, so `HttpDownloader` drives it through its own dedicated -//! `tokio::runtime::Runtime` instead (see `http_downloader::curl_runtime`), not through this -//! module. The other call sites (`file_downloader.rs`, `version_guesser.rs`, -//! `installation_manager.rs`, `composer_repository.rs`, `sync_helper.rs`) still rely on this -//! module because none of their awaited futures actually park on a reactor. +//! `main.rs` enters a single top-level `tokio::runtime::Runtime` for the whole process, so in +//! production `block_on` below always finds that runtime ambiently available via +//! `Handle::try_current()` and rides it via `tokio::task::block_in_place` — this is the +//! tokio-sanctioned way to call `Handle::block_on` from sync code that may itself already be +//! running inside a task driven by that same runtime (a plain nested `Runtime::block_on` would +//! panic with "Cannot start a runtime from within a runtime"; `block_in_place` does not). +//! Because it rides the real ambient runtime instead of a reactor-less busy-spin, awaited futures +//! that actually need the reactor (timers, real non-blocking socket I/O such as +//! `CurlDownloader::download`) now work correctly here too. //! -//! Nesting `tokio` runtimes is forbidden ("Cannot start a runtime from within a runtime"), which is -//! why this no-reactor executor exists for those remaining call sites; it can be nested freely. +//! Test binaries generally call production sync APIs directly with no ambient runtime entered +//! (see `crates/shirabe/tests/common/async_runtime.rs` for the few that do enter one). For that +//! case — and for any other call site reached before `main.rs`'s runtime exists — `block_on` falls +//! back to a disposable single-threaded runtime scoped to just that one call. //! -//! TODO(phase-e): remove this module once the async bridges are either made genuinely synchronous or -//! consolidated onto a single shared runtime driven from `main`. +//! TODO(phase-e): this still leaves every one of `block_on`'s call sites synchronous rather than +//! genuinely `async fn` propagated up to `Command::execute`, which remains the end goal of the +//! async re-architecture (see the design doc). Nested `block_on` call sites (a sync fn reached +//! from inside another `block_on`'s async block) do not run concurrently with their siblings — +//! `block_in_place` only prevents panics/hangs there, it does not parallelize them — so real +//! overlap only exists where a call chain is genuinely `async fn`/`.await` end-to-end (as arranged +//! for `ComposerRepository::get_security_advisories`/`load_async_packages`, see +//! `repository/composer_repository.rs`). Closing that gap for the remaining call sites requires +//! the full `async fn` propagation this module was always meant to be replaced by. use std::future::Future; -use std::task::{Context, Poll}; -/// Polls `fut` to completion on the current thread without any tokio runtime. +/// Drives `fut` to completion, riding the ambient tokio runtime if one is entered (the normal +/// case once `main.rs` has started), or a disposable one-off runtime otherwise. /// -/// Relies on the invariant that no awaited future parks on a reactor (it would otherwise spin). +/// The fallback runtime is `multi_thread` (with a single worker), not `current_thread`: a nested +/// `block_on` call reached from inside `fut` (e.g. a sync fn deep in the same call chain hitting +/// this function again) needs `block_in_place`, which panics on a `current_thread` runtime. pub fn block_on(fut: F) -> F::Output { - let mut fut = std::pin::pin!(fut); - let waker = std::task::Waker::noop(); - let mut cx = Context::from_waker(waker); - loop { - if let Poll::Ready(value) = fut.as_mut().poll(&mut cx) { - return value; - } - std::hint::spin_loop(); + match tokio::runtime::Handle::try_current() { + Ok(_) => tokio::task::block_in_place(|| tokio::runtime::Handle::current().block_on(fut)), + Err(_) => tokio::runtime::Builder::new_multi_thread() + .worker_threads(1) + .enable_all() + .build() + .expect("failed to build a fallback runtime for sync_executor::block_on") + .block_on(fut), } } -- cgit v1.3.1