Repository navigation
audit: sync code that could be async in fbuild-cli + fbuild-python (sub-issue of #813) #817
Description
Sub-issue of #813. Audit of crates/fbuild-cli/ and crates/fbuild-python/ for synchronous code paths that could be migrated to async so they share the tokio runtime (CLI's per-invocation runtime, or pyo3-async-runtimes' tokio for Python).
Intro
These two crates have very different audiences but share the same goal: lean on tokio so they can do more than one I/O thing at a time.
-
fbuild-cli— transient CLI process; spins aBuilder::new_multi_thread()runtime inmain.rsandblock_on(cli::async_main()). Most of the CLI is already async (daemon_client.rs'sDaemonClientis fullyreqwest::Client+await, daemon spawn usestokio::process::Command, daemon polling usestokio::time::sleep). A few diagnostic and edge-case paths still use blocking primitives even though they execute inside the tokio context. The biggest motivator to fix them is concurrent progress reporting (e.g. spinner while a build streams) — today the CLI is already async-clean enough that a sync HTTP call in a hot path is the exception, not the rule. -
fbuild-python— PyO3 extension shipped to FastLED. The crate is structured around the "additive async surface" pattern (issue feat(python): native async API via pyo3-async-runtimes (AsyncSerialMonitor, AsyncDaemon) #65): sync types (SerialMonitor,Daemon,DaemonConnection) stay untouched for backwards compat and the native async counterparts (AsyncSerialMonitor,AsyncDaemon,AsyncDaemonConnection) wrap the same logic inpyo3_async_runtimes::tokio::future_into_py. The sync surface is intentional — but a few HTTP calls inside the sync surface (notablySerialMonitor.reset_device) still usereqwest::blocking::*even though the type owns atokio::runtime::Runtimealready and could reuse it.
PyO3-async-runtimes adoption status
I counted 16 pyo3_async_runtimes::tokio::future_into_py callsites across the crate:
async_serial_monitor.rs— 7 (__aenter__,__aexit__,read_lines,write,write_json_rpc, plus 2 more). Full async surface.async_daemon_connection.rs— 8 (__aenter__,__aexit__,build,deploy,monitor,build_result,deploy_result,monitor_result). Full async surface.daemon.rs— 3 onAsyncDaemon(status,ensure_running,stop). Full async surface.
Sync surface (SerialMonitor, DaemonConnection, Daemon) is entirely synchronous and uses either rt.block_on(...) (when the type owns a tokio::runtime::Runtime) or reqwest::blocking::* directly.
Migration order recommendation: the async surface is already complete and FastLED should consume it. The remaining work is purely internal hygiene on the sync surface — finding/replacing the few reqwest::blocking::* calls that could reuse the owned runtime via rt.block_on(reqwest::Client::...). That gives a single HTTP connection pool per SerialMonitor instance instead of a fresh blocking client per call (zero behavioral change visible to Python, but avoids re-running TLS / connection setup on every reset_device etc.).
For Daemon (the sync sibling of AsyncDaemon), the duplication is more substantial — there are two parallel _blocking / _async helpers (verify_broker_daemon_cache_identity_blocking vs _async, ensure_running_via_broker_blocking vs _async). If we're willing to make the sync Daemon methods spin a one-shot tokio::runtime::Builder::new_current_thread() and block_on(...) the async version, we delete ~80 lines of duplicated broker-adoption logic. Out of scope for this issue, but worth noting in #813.
Findings table
| Severity | Crate | File:line | Current pattern | Proposed async replacement |
|---|---|---|---|---|
| HIGH | fbuild-python | serial_monitor.rs:461 (reset_device) |
reqwest::blocking::Client::new().post(&url)...send() inside a #[pymethod] that already owns self.runtime: Option<Runtime> |
Use rt.block_on(reqwest::Client::new().post(&url)...send().await); eliminates a second runtime spinup per reset and reuses the existing connection pool. |
| HIGH | fbuild-python | serial_monitor.rs:493, serial_monitor.rs:516 (reset_device) |
std::thread::sleep(...) for USB re-enumeration and post-reset settle |
These run after py.allow_threads is implicitly NOT held; they're fine for sync SerialMonitor but the equivalent path in AsyncSerialMonitor should be tokio::time::sleep(...).await. Verify AsyncSerialMonitor::reset_device already does. (It does — async_serial_monitor.rs:287 uses reqwest::Client::new(); this finding is to align the sync path's choice of HTTP client.) |
| HIGH | fbuild-python | daemon.rs:142 (verify_broker_daemon_cache_identity_blocking) |
reqwest::blocking::Client::builder()...send()...json() |
Already has an _async counterpart at daemon.rs:158. Migrate sync Daemon::ensure_running (line 283) to one-shot block_on(_async) and delete the blocking helper. |
| HIGH | fbuild-python | daemon.rs:181-220 (ensure_running_via_broker_blocking) |
Sync clone of ensure_running_via_broker_async (lines 222-267); uses reqwest::blocking::get and std::thread::sleep in the health-poll loop |
Delete the sync clone; have sync Daemon::ensure_running create a one-shot current_thread runtime and block_on(ensure_running_via_broker_async(...)). ~80 LoC of duplication goes away. |
| HIGH | fbuild-python | daemon.rs:191-201 (broker health poll) |
for _ in 0..100 { std::thread::sleep(100ms); reqwest::blocking::get(url) } |
Replaced by the change above — the async version already polls with tokio::time::sleep + reqwest::Client. |
| HIGH | fbuild-python | daemon.rs:291,327 (Daemon::ensure_running direct path) |
reqwest::blocking::get(&url) for /health checks + std::thread::sleep(100ms) polling |
Same as above — fold into a single block_on(async path). |
| HIGH | fbuild-python | daemon.rs:339-345 (Daemon::stop) |
reqwest::blocking::Client::new().post(&url).headers(...).send() |
AsyncDaemon::stop (line 505) already exists as async. Sync version should block_on(async_version) instead of cloning the body. |
| HIGH | fbuild-python | daemon.rs:350 (Daemon::status) |
reqwest::blocking::get(&url) + resp.text() blocking |
Fold into one-shot block_on(AsyncDaemon::status_impl(...)). |
| HIGH | fbuild-python | outcome.rs:121-164 (send_op) |
Whole function is reqwest::blocking::Client POST + JSON. Used by DaemonConnection::{build, deploy, monitor} |
send_op_async already exists in the same file (line 172). Sync send_op could literally be tokio::runtime::Builder::new_current_thread()...block_on(send_op_async(...)) — 35 lines deleted. |
| MEDIUM | fbuild-python | serial_monitor.rs:6,33-35 (SerialMonitor struct holds Mutex<WsSink> + Mutex<WsSource> from std::sync) |
std::sync::Mutex cannot be held across .await, so each WS op must lock() outside the future and pass into block_on. Works today, but forces every WS op to be sync. |
(Already done in AsyncSerialMonitor — it uses Arc<tokio::sync::Mutex<Option<_>>> deliberately, per the module docstring at line 22-42.) This finding is informational; sync SerialMonitor cannot adopt tokio::sync::Mutex without becoming async. |
| MEDIUM | fbuild-cli | cli/port_scan.rs:117-160 (fetch_overlay_to) |
reqwest::blocking::Client for USB VID overlay fetch, wrapped in std::thread::spawn(...).join() to escape the outer tokio runtime — comment explicitly says "reqwest::blocking spins its own internal runtime and rejects being called from inside an outer tokio runtime" |
Migrate to reqwest::Client::builder().timeout(15s).build() then .get(url).send().await — the surrounding port_scan is invoked from the async dispatcher so no thread hop is needed. Removes the workaround thread and lets cancellation propagate. |
| MEDIUM | fbuild-cli | cli/show.rs:38-58 (show_daemon_logs follow loop) |
std::thread::sleep(100ms) polling loop + std::fs::File::open + seek + read_to_string |
Use tokio::time::interval(Duration::from_millis(100)) and tokio::fs::*. Lets Ctrl+C / signal handling work cleanly under the tokio runtime. Low-throughput so the gain is small but the consistency win is real. |
| MEDIUM | fbuild-cli | daemon_client.rs:876-887 (spawn_daemon_process) |
std::fs::create_dir_all + std::fs::OpenOptions::new().create(true).append(true).open(...) for the daemon log file inside an async fn |
tokio::fs::create_dir_all(...).await + tokio::fs::OpenOptions::new().append(true)...open().await. Doesn't matter for a one-shot 1-byte create, but keeps the function fully non-blocking. |
| MEDIUM | fbuild-cli | cli/clang_tools.rs:274,299,369 (IWYU cache read/write) |
std::fs::read_to_string, std::fs::write, std::fs::read inside tokio::spawn tasks |
These tasks already run tokio::process::Command::output().await for IWYU itself, so the fs ops are tiny compared to the subprocess. Migrating to tokio::fs::* is purely a consistency win, not a perf one. |
| MEDIUM | fbuild-cli | daemon_client.rs:166-179 (LAST_DAEMON_ACQUISITION) |
OnceLock<Mutex<Option<DaemonAcquisition>>> using std::sync::Mutex |
The acquisition struct is read/written from async fns but the lock is never held across an .await, so std::sync::Mutex is fine. Leaving as-is is correct. (Listed for completeness — this is NOT a finding.) |
| LOW | fbuild-cli | main.rs:6-27 |
Trampoline-thread + tokio::runtime::Builder::new_multi_thread().build()...block_on(cli::async_main()) |
This is the runtime entry point; not a finding. Listed only so #813 readers don't flag it. |
| LOW | fbuild-cli | cli/lnk.rs:112,168,197 |
std::fs::read, std::fs::File::create + write_all for .lnk archive read/write inside async fns |
Consistency-only; the surrounding download_file().await already does the heavy lifting. |
| LOW | fbuild-cli | cli/clangd_config.rs:63-87,148,224 |
std::fs::write, std::fs::read_to_string for .clangd / .vscode/settings.json generation |
One-shot config file emission; the function is itself sync (fn run_clangd_config(...)), no async runtime to migrate to. Out of scope unless the surrounding command itself is made async. |
| LOW | fbuild-cli | cli/purge.rs:104-174, cli/symbols_cmd.rs:107-180, cli/graph_cmd.rs:85, cli/build.rs:160 |
Various std::fs::* calls in sync command bodies |
These commands are inherently sync (file system enumeration, ELF parsing, etc.); migrating to tokio::fs::* would force them to be async without measurable benefit. Skip. |
| LOW | fbuild-cli | mcp/server.rs:24-33 |
std::io::stdin().lock(); for line in reader.lines() { ... } blocking stdin read loop inside an async fn run_mcp_server() -> i32 |
MCP protocol is request/response over stdin/stdout; one outstanding request at a time. Migrating to tokio::io::stdin() adds complexity for zero behavioral gain. Skip unless we want to add concurrent notification streaming. |
What was searched
Grep patterns (across both crates/fbuild-cli/src/ and crates/fbuild-python/src/):
reqwest::— classifiedreqwest::Client(async, good) vsreqwest::blocking::*(sync, candidate)block_on— distinguished the explicit sync-Python bridge calls (intentional, in PyO3 entry points) fromblock_oninside HTTP/WS plumbing (candidate for sharing the host runtime)pyo3_async_runtimes— counted the existing async surface (16 callsites, all inAsync*types)#[pyfunction]/#[pymethods]/#[staticmethod]— enumerated PyO3 entry points, sorted by sync vs async return typestd::thread::sleep/std::thread::spawn— flagged blocking sleeps insideasynccontextsstd::process::Commandvstokio::process::Command— CLI uses tokio variant everywhere except the diagnostic clang tools (which use tokio); fbuild-python usesstd::process::Commandfor daemon spawn, which is correct since the child is detachedtokio_tungstenite::/tungstenite::— only infbuild-python(syncSerialMonitorusesblock_on(read.next());AsyncSerialMonitoruses native.await)std::sync::Mutex/RwLock/mpsc— all uses are either short-lived non-.awaitcritical sections or test infrastructuretokio::runtime::Runtime::new()— syncSerialMonitorconstructs a per-instance multi-thread runtime; tests construct one-shot runtimes. PyO3 itself uses the sharedpyo3-async-runtimesruntime for async surface, as expectedClient::new/Client::builder/Client::default— daemon_client.rs's singlereqwest::ClientperDaemonClientinstance is correct (one connection pool per CLI invocation)spawn_blocking/block_in_place— no occurrences in fbuild-cli or fbuild-python (correctly — fbuild-cli has no blocking work; fbuild-python sync surface usesRuntime::block_ondirectly)WebSocket readsfrom/ws/serial-monitor,/ws/logs,/ws/status— onlyfbuild-pythonreads/ws/serial-monitor(SerialMonitorsync viablock_on(read.next()),AsyncSerialMonitorasync via.await)
Modules / files audited:
fbuild-cli (12 files): main.rs, daemon_client.rs (+ daemon_client/tests.rs, daemon_client/types.rs), lib_select.rs, mcp/server.rs, mcp/jsonrpc.rs, cli/dispatch.rs, cli/daemon_cmd.rs, cli/port_scan.rs, cli/show.rs, cli/clang_tools.rs, cli/clangd_config.rs, cli/lnk.rs, cli/serial_probe.rs, cli/symbols_cmd.rs, cli/purge.rs, cli/graph_cmd.rs, cli/build.rs, cli/compile_many.rs, cli/bringup.rs.
fbuild-python (9 files): lib.rs, daemon.rs, daemon_connection.rs, async_daemon_connection.rs, serial_monitor.rs, async_serial_monitor.rs, outcome.rs, json_rpc.rs, messages.rs.
Out-of-scope notes
-
block_onat the PyO3 boundary in sync types is intentional, per feat(python): native async API via pyo3-async-runtimes (AsyncSerialMonitor, AsyncDaemon) #65's additive-async pattern. The crate explicitly preserves syncSerialMonitor,DaemonConnection,Daemonfor backwards-compatible Python consumers. We are not proposing to delete those types — only to thin out the sync internal helpers that duplicate logic already present in theirAsync*siblings. -
std::process::Command::spawnfor daemon spawn in bothfbuild-cli/src/daemon_client.rs::spawn_daemon_process(uses tokio) andfbuild-python/src/daemon.rs::Daemon::ensure_running(uses std) is intentional. The daemon is detached and outlives its parent (#32); tokio vs stdCommanddoesn't matter for aStdio::null()+spawn()+ no.awaitpath. -
The
LAST_DAEMON_ACQUISITIONOnceLock<Mutex<Option<_>>>indaemon_client.rs:166is correctlystd::sync::Mutex— the lock is never held across.await, only for set/get of a small struct. -
CLI WebSocket consumption does not exist. The CLI does not connect to
/ws/serial-monitor,/ws/logs, or/ws/statusdirectly — the only WebSocket consumer in this audit's scope isfbuild-python'sSerialMonitor/AsyncSerialMonitor, which talk to/ws/serial-monitor. The streaming build output uses NDJSON over HTTP (daemon_client.rs:304), not a WebSocket. -
Test-only
tokio::runtime::Runtime::new()constructions inlib.rs:312,330,350are integration-test helpers and not part of the runtime story. -
fbuild-cli/src/cli/show.rs::show_daemon_logsis invoked as a syncfnfrom the dispatcher (it has noasync fnwrapping). Migrating it totokio::fsrequires also making the caller async, which is straightforward but touchesdispatch.rs. Listed as MEDIUM rather than HIGH because the throughput is human-paced log tailing.
Activity
fix(async): close audit gaps from #813 sub-issues #815/#817/#818 (#842)