kj-rs: rework the async bridge waker/event machinery - #7010
Conversation
|
APIError: Invalid Anthropic API Key |
2 similar comments
|
APIError: Invalid Anthropic API Key |
|
APIError: Invalid Anthropic API Key |
|
@danlapid Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7010 +/- ##
==========================================
- Coverage 67.79% 67.75% -0.04%
==========================================
Files 468 468
Lines 132275 132339 +64
Branches 21474 21474
==========================================
Hits 89671 89671
- Misses 29528 29593 +65
+ Partials 13076 13075 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| // only (see requireCurrentOrTearingDown below), so the exception cost is irrelevant. | ||
| const kj::Executor* current = nullptr; | ||
| auto maybeException = | ||
| kj::runCatchingExceptions([&]() { current = &kj::getCurrentThreadExecutor(); }); |
There was a problem hiding this comment.
surely we can add utility method for you!
| @@ -0,0 +1,144 @@ | |||
| // Regression test: FutureWakerCell "neutralize-on-drop". | |||
There was a problem hiding this comment.
I don't think this file tests anything? outside of this test?
| @@ -0,0 +1,321 @@ | |||
| // Regression test: one shared kj Event as the onReady target of MANY concurrent pending nodes. | |||
There was a problem hiding this comment.
this too doesn't seem to be kj-rs specific?
1b12cd5 to
dbc8012
Compare
Groundwork for the tokio-backed Rust I/O backend (kj-rs-tokio / kj-rs-io, landing separately); kj-rs itself stays a pure Promise<->Future bridge. - Replace KjWaker with FutureWakerCell: every cloned waker is a same-thread cell (non-atomic kj::Refcounted; the bridge's single-thread axiom) that arms the owning FuturePollEvent directly via Event::armDepthFirst(). The cell's link to the event is weak and structurally invalidated when the event dies, so wakers Rust retains past the future's lifetime (e.g. parked in a channel's AtomicWaker) neutralize into safe no-ops instead of arming a freed event. Waker ownership round-trips through RawWaker data slots via kj::Rc::disown()/reown() -- hence the capnp-cpp pin bump to the current v2 head, which carries those (merged upstream). - Replace the LinkedGroup machinery (linked-group.h + test) with an intrusive weak link (RustPromiseAwaiter::link / FuturePollEvent::leaves). - Make bridged kj::Promise<T>s eager by default: the Rust future is polled to its first suspension point at conversion, so KJ callers no longer need a manual .eagerlyEvaluate(nullptr); RustFuture::lazily() is the escape hatch for the rare cold case. - Convert panics escaping a bridged future's poll into kj::Exceptions (a rejected promise) instead of aborting the process, mirroring the sync bridge's catch_unwind path. - Add a thread-local armed-waker hook so an integrating kj::EventPort that drives tokio tasks inside its own wait() (the upcoming kj-rs-tokio) can nudge itself out of a blocking park; null/no-op by default. - Split the cxx bridge module out of lib.rs into ffi.rs, and quarantine unsafe into named FFI islands: deny(unsafe_code) crate-wide, re-allowed per-module only where the FFI seam genuinely needs it. - Depend on @capnp-cpp//src/kj:kj-async-core instead of the :kj-async umbrella, keeping kj-rs (and everything built on it) off the concrete kj OS event loop. - Qualify bare uint as kj::uint in src/rust/kj/tests/ffi-test.c++: these dependency changes make that target newly compile on Windows CI, where no global uint exists (POSIX gets one from sys/types.h). - New tests: waker neutralization across event death (neutralize-waker-test), FuturePollEvent shared-event semantics (shared-event-test), and expanded future/awaiter coverage.
dbc8012 to
731a392
Compare
| // Note: Implementing these traits does not seem to be required for building, but the Waker | ||
| // documentation makes it clear Send and Sync are a requirement of the pointed-to type. | ||
| // Thread-safety: `std::task::Waker` documents that the vtable functions must be thread-safe. | ||
| // The bridge is single-threaded — no waker is ever woken, cloned, or dropped from another thread |
There was a problem hiding this comment.
no waker is ever woken, cloned, or dropped from another thread
As far as I can tell, nothing prevents safe Rust from doing exactly this. So, every time we write new async code, or depend on some other library which exposes an async interface, we'll have to audit it for cross-thread waker usage, or risk memory unsafety in allegedly safe. This undermines one of the main reasons why we want to adopt Rust.
The comment's statement is also not true if we include our internal codebase, where we already depend on cross-thread wakers. That code will suddenly become UB if we merge this PR as-is.
The existing implementation was already correct, and optimized for the single-threaded case if the waker wasn't cloned. What motivated this change?
There was a problem hiding this comment.
or risk memory unsafety in allegedly safe
"allegedly safe code." GitHub won't let me edit the comment for some reason.
5 participants
Groundwork for the tokio-backed Rust I/O backend (kj-rs-tokio / kj-rs-io, landing separately); kj-rs itself stays a pure Promise<->Future bridge.