Repository navigation
Log a panic in a Rust JSG resource's Drop instead of aborting - #8400
littledivy wants to merge 1 commit into
Conversation
A panic in a Rust JSG resource's `Drop` aborted the process. The
resource is dropped from `rust::jsg::Wrappable::~Wrappable`, which
cppgc runs during its sweep. The cxx bridge turns the panic into a
`kj::Exception`, and throwing it out of the destructor called
`std::terminate`. A panic in a method, by contrast, is caught and
becomes a JS internal error, so one bad `Drop` could take down every
isolate in the process.
In this patch `~Wrappable` catches the exception and logs it with
`LOG_EXCEPTION("rustResourceDropPanic", ...)`, and the process keeps
running. Every way a resource is released goes through `~Wrappable`,
so this covers the GC sweep, the last C++ reference being released and
isolate teardown. Rust still drops the remaining fields and frees the
allocation while the panic unwinds. The README notes the behaviour.
Only a path that used to abort changes, and no JS API changes, so this
has no compatibility flag.
There was a problem hiding this comment.
✅ Approved — This is a small, well-scoped fix. The destructor catches and logs the exception the cxx bridge turns a Drop panic into, a test covers the GC-sweep path, and no issues were found.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
📋 Code Quality: ✅ LGTM
~Wrappable now wraps wrappable_invoke_drop in kj::runCatchingExceptions and logs a caught exception with LOG_EXCEPTION. The trait object is cleared before drop_rc(), so nothing after the catch depends on the drop having finished. The new code uses only KJ and LOG_EXCEPTION, not C++ JSG. The new test checks that the drop runs exactly once, during the GC sweep.
📝 Documentation: ✅ LGTM
The README paragraph describes the current behaviour accurately: a panic in a resource's Drop is logged and the process keeps running.
Reviewed commit: 1dc056c3
A panic in a Rust JSG resource's
Dropaborted the process. The resource is dropped fromrust::jsg::Wrappable::~Wrappable, which cppgc runs during its sweep. The cxx bridge turns the panic into akj::Exception, and throwing it out of the destructor calledstd::terminate. A panic in a method, by contrast, is caught and becomes a JS internal error. So one badDropcould take down every isolate in the process.In this patch
~Wrappablecatches the exception and logs it withLOG_EXCEPTION("rustResourceDropPanic", ...), and the process keeps running. Every way a resource is released goes through~Wrappable, so this covers the GC sweep, the last C++ reference being released and isolate teardown. Rust still drops the remaining fields and frees the allocation while the panic unwinds (std'sRc::drop_slowfrees it even whenT's destructor panics). The README notes the behaviour.Droppanics during a full GClibc++abi: terminating due to uncaught exception of type kj::Exceptionerror: ... panic in jsg::v8::ffi::wrappable_invoke_drop; sentryErrorContext = rustResourceDropPanic, test passesMeasured with the new
panic_in_drop_during_gc_sweeptest in//src/rust/jsg-test:jsg-test_test, before and after the change.Only a path that used to abort changes, and no JS API changes, so this has no compatibility flag.