diff options
| -rw-r--r-- | crates/shirabe/src/main.rs | 13 | ||||
| -rw-r--r-- | crates/shirabe/src/repository/composer_repository.rs | 6 | ||||
| -rw-r--r-- | crates/shirabe/src/util/http_downloader.rs | 25 | ||||
| -rw-r--r-- | crates/shirabe/src/util/sync_executor.rs | 61 |
4 files changed, 52 insertions, 53 deletions
diff --git a/crates/shirabe/src/main.rs b/crates/shirabe/src/main.rs index 8e8e3299..2ae6d903 100644 --- a/crates/shirabe/src/main.rs +++ b/crates/shirabe/src/main.rs @@ -8,6 +8,19 @@ fn main() { std::sync::LazyLock::force(&PHP_ENV); std::sync::LazyLock::force(&PHP_SERVER); + // The single process-wide tokio Runtime. `shirabe::run` and everything under it + // (Command::execute and friends) is still synchronous top to bottom; entering the runtime + // here (rather than driving `run` via `.block_on`) just makes it ambiently available via + // `Handle::try_current()` for `util::sync_executor::block_on`'s many scattered call sites, + // which ride it through `tokio::task::block_in_place` instead of each spinning up (or + // busy-spin-polling without) their own. See sync_executor.rs for the TODO(phase-e) tracking + // the eventual goal of propagating `async fn` all the way up to here instead. + let runtime = tokio::runtime::Builder::new_multi_thread() + .enable_all() + .build() + .expect("failed to build the top-level tokio runtime"); + let _runtime_guard = runtime.enter(); + let result = shirabe::run(std::env::args().collect()); let exit_code = match result { Ok(exit_code) => exit_code, diff --git a/crates/shirabe/src/repository/composer_repository.rs b/crates/shirabe/src/repository/composer_repository.rs index 1b04de39..f3f75ea2 100644 --- a/crates/shirabe/src/repository/composer_repository.rs +++ b/crates/shirabe/src/repository/composer_repository.rs @@ -3155,11 +3155,7 @@ impl ComposerRepository { } } - let response_result = self - .http_downloader - .borrow_mut() - .add(&filename, options) - .await; + let response_result = self.http_downloader.borrow().add(&filename, options).await; match response_result { Ok(response) => self.async_fetch_file_accept(response, &filename, cache_key), Err(e) => self.async_fetch_file_reject(e, &filename, cache_key, last_modified_time), diff --git a/crates/shirabe/src/util/http_downloader.rs b/crates/shirabe/src/util/http_downloader.rs index b7173c5e..c9e1e4a6 100644 --- a/crates/shirabe/src/util/http_downloader.rs +++ b/crates/shirabe/src/util/http_downloader.rs @@ -91,29 +91,6 @@ impl Default for HttpDownloaderMockHandler { } } -/// A single-threaded tokio Runtime used only to drive `CurlDownloader::download()` (which needs a -/// real reactor now that it uses the non-blocking `reqwest::Client`, unlike `sync_executor::block_on` -/// which assumes every awaited future resolves synchronously). `current_thread` is used because -/// `block_on` (unlike `spawn`) has no `Send` bound, and `download()`'s future closes over -/// `Rc<RefCell<...>>` handles that are not `Send`. -/// -/// `get()`/`copy()` bridge into the async core via `sync_executor::block_on` instead of this -/// Runtime (nesting this same Runtime's `block_on` inside itself, on the curl path, would panic -/// with "Cannot start a runtime from within a runtime"); this Runtime is only ever entered at the -/// single point where `CurlDownloader::download()` is awaited. -/// -/// TODO(phase-e): remove this once `HttpDownloader::add`/`get` are driven by `Loop::wait`'s -/// `FuturesUnordered` under a single top-level Runtime (see the async re-architecture design). -fn curl_runtime() -> &'static tokio::runtime::Runtime { - static RT: std::sync::LazyLock<tokio::runtime::Runtime> = std::sync::LazyLock::new(|| { - tokio::runtime::Builder::new_current_thread() - .enable_all() - .build() - .expect("failed to build the temporary CurlDownloader bridge runtime") - }); - &RT -} - impl HttpDownloader { /// @param IOInterface $io The IO instance /// @param Config $config The config @@ -346,7 +323,7 @@ impl HttpDownloader { } let curl = self.curl.as_ref().unwrap(); - return match curl_runtime().block_on(curl.download(&origin, url, options, copy_to)) { + return match curl.download(&origin, url, options, copy_to).await { Ok(Ok(response)) => Ok(response), Ok(Err(transport_exception)) => Err(transport_exception.into()), Err(e) => Err(e), 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<F: Future>(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), } } |