Skip to content

fix(runtime): do not panic when the driver is entered from async (CON-1) - #313

Open
fornwall wants to merge 1 commit into
mainfrom
con-1-async-context-block-on
Open

fix(runtime): do not panic when the driver is entered from async (CON-1)#313
fornwall wants to merge 1 commit into
mainfrom
con-1-async-context-block-on

Conversation

@fornwall

Copy link
Copy Markdown
Owner

The problem

The ADBC traits are synchronous while the Spanner client is async, so every driver call blocks the caller on the driver's own Tokio runtime. Tokio permits blocking only on a thread that is not currently running a runtime, and panics otherwise:

  • Call: any ADBC call or RecordBatchReader::next from a Tokio worker → "Cannot block the current thread from within a runtime".
  • Drop: a reader that is the last Arc<Runtime> holder, dropped on an async thread → "Cannot drop a runtime in a context where blocking is not allowed".

For the cdylib both unwind across the C FFI boundary and poison the driver handle. Both reproduce: reverting each half of this fix makes the new tests panic with exactly those two messages.

Why the suggested fix isn't used for the call half

CON-1 suggested detecting tokio::runtime::Handle::try_current() and returning a clean error advising spawn_blocking. That predicate is wrong. Probing every context a driver call can be reached from:

context Handle::try_current() block_on Runtime::drop
plain thread Err works works
multi-thread block_on body / task Ok panics panics
multi-thread spawn_blocking / block_in_place Ok works works
current-thread block_on body / task Ok panics panics
current-thread spawn_blocking Ok works works

Handle::try_current() is Ok in three contexts where blocking works — including spawn_blocking, the very workaround the error message would have to recommend. Under the default multi-thread #[tokio::main], spawn_blocking(|| stmt.execute()) would start returning InvalidState telling the caller to use spawn_blocking. That breaks working code, which is worse than the panic.

Tokio exposes no public API for the real predicate (context::try_enter_blocking_region, i.e. the EnterRuntime flag, is pub(crate)), so a "clean error" is not implementable correctly here.

The fix

Don't predict. runtime::block_on_bridged picks, per Handle::try_current().map(runtime_flavor), a construction legal in every context that flavour can present:

  • No runtime context (the ordinary sync ADBC caller) → plain block_on.
  • MultiThreadblock_in_place: on a worker it hands the core to another thread, so the caller's runtime keeps full capacity; on a spawn_blocking thread it's a pass-through. Legal either way, spawns nothing.
  • Anything elseCurrentThread, where block_in_place itself panics and a spawn_blocking thread is indistinguishable from the one worker, plus any future #[non_exhaustive] flavour → a std::thread::scope thread, which carries no Tokio context and may therefore always block (and needs no 'static).

Both bridge sites go through it: block_on_cancellable (covering SpannerBatchReader::next / BoundQueryBatchReader) and SpannerDatabase::connect's plain block_on. A panic from the operation's own future is still re-raised unchanged via resume_unwind.

Net effect: the driver works from async code rather than erroring. The caller's thread still blocks — inherent to a synchronous ADBC API, and what the caller asked for — so spawn_blocking remains the documented recommendation, just no longer a requirement.

The drop half is fixed exactly as suggested, because there the same over-approximation is safe: SharedRuntime is now Arc<DriverRuntime>, a newtype holding Option<Runtime> and Deref-ing to Runtime (no call site changed, no unsafe/ManuallyDrop), whose Drop uses shutdown_background() in a runtime context. A false positive only skips the wait for idle worker threads; it cannot turn a working call into an error.

Alternatives considered

  • Handle::try_current() guard + clean error (as suggested) — rejected: breaks spawn_blocking under #[tokio::main], see above.
  • catch_unwind around the panic — rejected: aborts the process under panic = "abort", and would print a panic + backtrace before every "clean" error.
  • block_in_place everywhere — rejected: panics on a current-thread runtime, hence the scoped-thread fallback.
  • Scoped thread everywhere — correct but needlessly costs a multi-thread caller a worker for the call's duration, which block_in_place avoids.

Collateral

block_on_cancellable now requires Send (the scoped-thread path needs it). One site needed adjusting: statistics.rs built its scan futures from a closure taking &PreparedTable, making it higher-ranked over that borrow and defeating the compiler's Send check ("implementation of FnOnce is not general enough" — a rustc limitation, not a real violation). The futures are now collected eagerly; each captures only owned data, and the sql was cloned per scan anyway. Async blocks are lazy, so no RPC fires earlier than before.

Verification

  • cargo fmt --all --check, cargo clippy --all-targets --all-features -- -D warnings, cargo test — all clean (391 tests).
  • RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --all-features — clean (CI enforces this).
  • scripts/with-emulator.sh cargo test --test integration — 52/52 pass, including get_statistics_reports_real_counts, which covers the restructured scan path.
  • New tests, each verified to fail (panic) with the fix reverted:
    • src/runtime.rs: a bridged call works from all six contexts in the table (the spawn_blocking rows pin down the rejected design); cancellation still works on the scoped-thread path; last-handle drop is panic-free in both flavours with a live task pending.
    • tests/mock_spanner.rs: driver_entered_from_an_async_context_does_not_panic drives new_connectionexecute → reader drain → drop against the mock server from inside both flavours — offline and end-to-end through the real ADBC entry points.

Docs updated: new "Calling from async code" crate-doc section in lib.rs, the runtime module docs, and CLAUDE.md's sync-over-async design point. REVIEW.md's CON-1 is ticked with a *Resolved:* note recording the evidence table and why the suggested predicate was not used.

This is mitigation, not a cure — the root cause remains adbc_core's synchronous trait surface (UP-13).

🤖 Generated with Claude Code

https://claude.ai/code/session_01QuTEmz4mFUC2J7ptHpqWwK

The ADBC traits are synchronous, so every driver call blocks the caller on
the driver's own Tokio runtime. Tokio permits blocking only on a thread that
is not currently *running* a runtime and panics otherwise — both on the call
("Cannot block the current thread from within a runtime") and on dropping the
runtime ("Cannot drop a runtime in a context where blocking is not allowed"),
which a streamed reader can trigger as the last `Arc` holder. For the cdylib
those panics unwind across the C FFI boundary and poison the driver handle.

CON-1 suggested guarding on `Handle::try_current()` and returning a clean
error advising `spawn_blocking`. Probing every reachable context shows that
predicate is wrong: `Handle::try_current()` is `Ok` inside `spawn_blocking`
and `block_in_place` too, where blocking is perfectly legal. A guard built on
it would reject the very workaround the error recommends, under the default
multi-thread `#[tokio::main]` — breaking working code. No public Tokio API
exposes the real predicate (the `EnterRuntime` flag is `pub(crate)`), and
`catch_unwind` is not an option: it aborts under `panic = "abort"`.

So do not predict. `runtime::block_on_bridged` picks, per runtime flavour, a
construction legal in *every* context that flavour can present: no context →
plain `block_on`; MultiThread → `block_in_place` (hands the core off on a
worker, pass-through on a `spawn_blocking` thread); anything else — including
CurrentThread, where `block_in_place` itself panics — → a `std::thread::scope`
thread, which carries no Tokio context and so may always block. Both bridge
sites use it: `block_on_cancellable` (covering the streamed readers) and
`SpannerDatabase::connect`. The driver now *works* from async code rather than
erroring; it still blocks the caller, as a synchronous API must, so
`spawn_blocking` stays the documented recommendation.

The drop half is fixed as suggested — `SharedRuntime` is now a
`Arc<DriverRuntime>` newtype that `Deref`s to `Runtime` and shuts down via
`shutdown_background()` in a runtime context. There the same
over-approximation is safe: it only skips a wait, and cannot turn a working
call into an error.

`block_on_cancellable` now requires `Send`, which forced building the
statistics scan futures eagerly — their closure's higher-ranked
`&PreparedTable` argument defeated the compiler's `Send` check.

Covered by unit tests over all six contexts (the `spawn_blocking` cases pin
down the rejected design), a cancellation test on the scoped-thread path, and
an end-to-end mock-server test driving connect → execute → drain → drop from
both flavours; each panicked before this change.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QuTEmz4mFUC2J7ptHpqWwK
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant