Conversation
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR reverts the requestTermination / isTerminationRequested mechanism from the C++ JSG layer, but the Rust FFI layer still depends on those removed methods.
Issues (by severity)
-
Build break (high): The Rust FFI layer in
src/rust/jsg/has active callers of the removedIsolateBase::requestTermination()andIsolateBase::isTerminationRequested()methods. These files need corresponding changes:src/rust/jsg/ffi.c++:1089-1095—isolate_request_termination()andisolate_is_termination_requested()call the removed methodssrc/rust/jsg/ffi.h:343-344— declares those FFI functionssrc/rust/jsg/v8.rs:555-556— Rust-side FFI declarationssrc/rust/jsg/lib.rs:599-623—Lock::request_termination()andLock::is_termination_requested()public methodssrc/rust/jsg/lib.rs:819—catch_panic()callslock.request_termination()after converting a Rust panic to a JS exceptionsrc/rust/jsg-test/tests/resource_callback.rs:441-488— test that asserts onis_termination_requested()
The
catch_paniccase is particularly important: it's the Rust panic handler that terminates the isolate after a panic to prevent GC from observing partially-mutated state. This needs a replacement — likely switching toterminateExecution()(which is still available onIsolateBase) instead of the removedrequestTermination(). Theis_termination_requested()check in the test would need a different assertion strategy since there's no longer a flag to query.
| // threads, without holding the lock. | ||
| // Immediately cancels JavaScript execution in this isolate, causing an uncatchable exception to | ||
| // be thrown. Safe to call across threads, without holding the lock. | ||
| void terminateExecution() const; |
There was a problem hiding this comment.
The C++ side is fine, but the Rust FFI layer still calls requestTermination() and isTerminationRequested() which no longer exist:
src/rust/jsg/ffi.c++:1089-1095— will fail to compilesrc/rust/jsg/lib.rs:819—catch_panic()callslock.request_termination()to terminate the isolate after a Rust panic, preventing GC from observing partially-mutated state. This is an important safety mechanism that needs a replacement (probablyterminateExecution()).src/rust/jsg-test/tests/resource_callback.rs— test asserts onis_termination_requested()
These all need to be updated as part of this revert.
|
Review posted successfully on PR #6626. Here's a summary: High-severity issue found: The PR removes
|
|
The generated output of |
A better solution was found in the form of https://chromium-review.googlesource.com/c/v8/v8/+/7734848 and so this interface is unused.