diff --git a/cpp/includes/RustArcPtr.h b/cpp/includes/RustArcPtr.h index 3057d6b..e873228 100644 --- a/cpp/includes/RustArcPtr.h +++ b/cpp/includes/RustArcPtr.h @@ -26,4 +26,78 @@ template <> struct Bridging { return uniffi_jsi::Bridging::toJs(rt, callInvoker, num); } }; + +/// Constructs a `DestructibleObject` that accepts a pointer and destructor +/// callback. This corresponds to a `UniffiRustArcPtr` object. The idea is that +/// this object has a C++ destructor which calls the Rust drop function +/// for the object. +/// +/// The JS object provides a `markDestroyed` function. This will override the +/// behaviour of the C++ destructor. +/// +/// This allows the generated JS to call the Rust object's `free` function +/// in the usual manner, including checks for panics, which are translated +/// into and thrown as JS errors. +/// +/// It should be noted that this is a hack: the C++ destructor. The relevant +/// comment on the HostObject's destructor is reproduced here: +/// +/// // The C++ object's dtor will be called when the GC finalizes this +/// // object. (This may be as late as when the Runtime is shut down.) +/// // You have no control over which thread it is called on. This will +/// // be called from inside the GC… +/// +/// Until hermes and React Native gain a `FinalizationRegistry`, this is +/// unlikely to get better. +class DestructibleObject : public jsi::HostObject { +private: + jsi::Runtime &rt; + std::mutex destroyedMutex; + bool destroyed = false; + uint64_t pointer; + std::function destructor; + +public: + DestructibleObject(jsi::Runtime &rt, uint64_t pointer, + std::function destructor) + : jsi::HostObject(), rt(rt), pointer(pointer), destructor(destructor) {} + + ~DestructibleObject() { + /// You can use uniffiDestroy() from JS instead of relying on GC. + /// + /// From + /// https://github.com/facebook/hermes/issues/982#issuecomment-1771325667: + /// + /// "Generally speaking, when dealing with native resources, it is + /// recommended to free the native resource explicitly when you are done + /// with it, because garbage collection is unpredictable and should not be + /// relied on for freeing native resources timely." + destroy_object(); + } + + void destroy_object() { + std::lock_guard lock(destroyedMutex); + if (!destroyed) { + destroyed = true; + destructor(pointer); + } + } + + jsi::Value get(jsi::Runtime &rt, const jsi::PropNameID &name) override { + std::string propName = name.utf8(rt); + if (propName == "markDestroyed") { + auto func = jsi::Function::createFromHostFunction( + rt, jsi::PropNameID::forAscii(rt, "markDestroyed"), 0, + [this](jsi::Runtime &rt, const jsi::Value &thisVal, + const jsi::Value *args, size_t count) -> jsi::Value { + std::lock_guard lock(this->destroyedMutex); + this->destroyed = true; + return jsi::Value::undefined(); + }); + return jsi::Value(rt, func); + } + return jsi::Value::undefined(); + } +}; + } // namespace uniffi_jsi diff --git a/crates/ubrn_bindgen/src/bindings/react_native/gen_cpp/mod.rs b/crates/ubrn_bindgen/src/bindings/react_native/gen_cpp/mod.rs index 4d5eade..ab8f569 100644 --- a/crates/ubrn_bindgen/src/bindings/react_native/gen_cpp/mod.rs +++ b/crates/ubrn_bindgen/src/bindings/react_native/gen_cpp/mod.rs @@ -9,7 +9,7 @@ use crate::bindings::{ metadata::ModuleMetadata, react_native::{ ComponentInterfaceExt, FfiArgumentExt, FfiCallbackFunctionExt, FfiFieldExt, FfiStructExt, - FfiTypeExt, + FfiTypeExt, ObjectExt, }, }; use anyhow::Result; diff --git a/crates/ubrn_bindgen/src/bindings/react_native/gen_cpp/templates/ObjectHelper.cpp b/crates/ubrn_bindgen/src/bindings/react_native/gen_cpp/templates/ObjectHelper.cpp new file mode 100644 index 0000000..d248ce3 --- /dev/null +++ b/crates/ubrn_bindgen/src/bindings/react_native/gen_cpp/templates/ObjectHelper.cpp @@ -0,0 +1,18 @@ +{%- import "macros.cpp" as cpp %} + +{%- for obj in ci.object_definitions() %} +{%- let bless = obj.ffi_function_bless_pointer() %} +{%- let free = obj.ffi_object_free() %} +{%- let uint64 = FfiType::UInt64 %} +{%- call cpp::cpp_fn_from_js_decl(bless) %} { + auto pointer = {{ uint64|bridging_class(ci) }}::fromJs(rt, callInvoker, args[0]); + auto static destructor = [](uint64_t p) { + auto pointer = reinterpret_cast(static_cast(p)); + RustCallStatus status = {0}; + {{ free.name() }}(pointer, &status); + }; + auto ptrObj = std::make_shared<{{ ci.cpp_namespace_includes() }}::DestructibleObject>(rt, pointer, destructor); + auto obj = jsi::Object::createFromHostObject(rt, ptrObj); + return jsi::Value(rt, obj); +} +{%- endfor %} diff --git a/crates/ubrn_bindgen/src/bindings/react_native/gen_cpp/templates/VTableCallbackFunction.cpp b/crates/ubrn_bindgen/src/bindings/react_native/gen_cpp/templates/VTableCallbackFunction.cpp index 196a37a..7336f01 100644 --- a/crates/ubrn_bindgen/src/bindings/react_native/gen_cpp/templates/VTableCallbackFunction.cpp +++ b/crates/ubrn_bindgen/src/bindings/react_native/gen_cpp/templates/VTableCallbackFunction.cpp @@ -117,6 +117,19 @@ namespace {{ ns }} { {%- if callback.has_rust_call_status_arg() -%} , RustCallStatus* uniffi_call_status {%- endif -%}) { + // If the runtime has shutdown, then there is no point in trying to + // call into Javascript. BUT how do we tell if the runtime has shutdown? + // + // Answer: the module destructor calls into callback `cleanup` method, + // which nulls out the rsLamda. + // + // If rsLamda is null, then there is no runtime to call into. + if (rsLambda == nullptr) { + // This only occurs when destructors are calling into Rust free/drop, + // which causes the JS callback to be dropped. + return; + } + // The runtime, the actual callback jsi::funtion, and the callInvoker // are all in the lambda. rsLambda( diff --git a/crates/ubrn_bindgen/src/bindings/react_native/gen_cpp/templates/wrapper.cpp b/crates/ubrn_bindgen/src/bindings/react_native/gen_cpp/templates/wrapper.cpp index 164bf98..c0faf51 100644 --- a/crates/ubrn_bindgen/src/bindings/react_native/gen_cpp/templates/wrapper.cpp +++ b/crates/ubrn_bindgen/src/bindings/react_native/gen_cpp/templates/wrapper.cpp @@ -144,6 +144,7 @@ void {{ module_name }}::set(jsi::Runtime& rt, const jsi::PropNameID& name, const } {%- include "StringHelper.cpp" %} +{%- include "ObjectHelper.cpp" %} // Methods calling directly into the uniffi generated C API of the Rust crate. {%- for func in ci.iter_ffi_functions_js_to_rust() %} diff --git a/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/mod.rs b/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/mod.rs index 4b657ee..5c639fa 100644 --- a/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/mod.rs +++ b/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/mod.rs @@ -30,7 +30,7 @@ use uniffi_meta::{AsType, ExternalKind}; use crate::bindings::metadata::ModuleMetadata; use crate::bindings::react_native::{ - ComponentInterfaceExt, FfiCallbackFunctionExt, FfiFunctionExt, FfiStructExt, + ComponentInterfaceExt, FfiCallbackFunctionExt, FfiFunctionExt, FfiStructExt, ObjectExt, }; #[derive(Default)] diff --git a/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/oracle.rs b/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/oracle.rs index afc3846..df9bedd 100644 --- a/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/oracle.rs +++ b/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/oracle.rs @@ -129,6 +129,7 @@ impl CodeOracle { pub(crate) fn ffi_type_label_for_cpp(&self, ffi_type: &FfiType) -> String { match ffi_type { + FfiType::RustArcPtr(_) => "UniffiRustArcPtr".to_string(), FfiType::ForeignBytes => "ArrayBuffer".to_string(), FfiType::RustBuffer(_) => "string".to_string(), _ => self.ffi_type_label(ffi_type), diff --git a/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/templates/ObjectTemplate.ts b/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/templates/ObjectTemplate.ts index 1e2eb78..a078aa3 100644 --- a/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/templates/ObjectTemplate.ts +++ b/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/templates/ObjectTemplate.ts @@ -13,6 +13,7 @@ {%- macro private_ctor() %} private constructor(pointer: UnsafeMutableRawPointer) { super(); + this.__rustPointer = pointer; this.__rustArcPtr = {{ obj_factory }}.bless(pointer); } {%- endmacro %} @@ -33,6 +34,7 @@ export class {{ impl_class_name }} extends AbstractUniffiObject implements {{ pr private __uniffiTypeName = "{{ impl_class_name }}"; private __rustArcPtr: UniffiRustArcPtr; + private __rustPointer: UnsafeMutableRawPointer; {%- match obj.primary_constructor() %} {%- when Some with (cons) %} @@ -65,7 +67,8 @@ export class {{ impl_class_name }} extends AbstractUniffiObject implements {{ pr uniffiDestroy(): void { if ((this as any).__rustArcPtr) { const pointer = {{ obj_factory }}.pointer(this); - this.__rustArcPtr.d(pointer); + {{ obj_factory }}.freePointer(pointer); + this.__rustArcPtr.markDestroyed(); delete (this as any).__rustArcPtr; } } @@ -106,21 +109,24 @@ export class {{ impl_class_name }} extends AbstractUniffiObject implements {{ pr const {{ obj_factory }}: UniffiObjectFactory<{{ type_name }}> = { create(pointer: UnsafeMutableRawPointer): {{ type_name }} { const instance = Object.create({{ impl_class_name }}.prototype); + instance.__rustPointer = pointer; instance.__rustArcPtr = this.bless(pointer); return instance; }, bless(p: UnsafeMutableRawPointer): UniffiRustArcPtr { - const d = this.freePointer; - return { p, d }; + return rustCall( + /*caller:*/ (status) => + nativeModule().{{ obj.ffi_function_bless_pointer().name() }}(p, status), + /*liftString:*/ FfiConverterString.lift + ); }, pointer(obj: {{ type_name }}): UnsafeMutableRawPointer { - const ptr = (obj as any).__rustArcPtr; - if (ptr === undefined) { + if ((obj as any).__rustArcPtr === undefined) { throw new UniffiInternalError.UnexpectedNullPointer(); } - return ptr.p; + return (obj as any).__rustPointer; }, clonePointer(obj: {{ type_name }}): UnsafeMutableRawPointer { diff --git a/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/templates/macros.ts b/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/templates/macros.ts index 9e273da..ebef872 100644 --- a/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/templates/macros.ts +++ b/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/templates/macros.ts @@ -93,6 +93,7 @@ super(); const pointer = {% call to_ffi_method_call(obj_factory, callable) %}; + this.__rustPointer = pointer; this.__rustArcPtr = {{ obj_factory }}.bless(pointer); } {%- endmacro %} diff --git a/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/templates/wrapper-ffi.ts b/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/templates/wrapper-ffi.ts index bee4a99..b1df49c 100644 --- a/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/templates/wrapper-ffi.ts +++ b/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/templates/wrapper-ffi.ts @@ -1,6 +1,7 @@ import { type StructuralEquality as UniffiStructuralEquality, type UniffiReferenceHolder, + type UniffiRustArcPtr, type UniffiRustCallStatus, type UniffiRustFutureContinuationCallback as RuntimeUniffiRustFutureContinuationCallback, } from 'uniffi-bindgen-react-native'; diff --git a/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/templates/wrapper.ts b/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/templates/wrapper.ts index 560999a..ffcd546 100644 --- a/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/templates/wrapper.ts +++ b/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/templates/wrapper.ts @@ -1,5 +1,6 @@ // This file was autogenerated by some hot garbage in the `uniffi` crate. // Trust me, you don't want to mess with it! +import { console } from "@/hermes"; import nativeModule, { {%- for def in ci.iter_ffi_definitions_exported_by_ts() %} diff --git a/crates/ubrn_bindgen/src/bindings/react_native/mod.rs b/crates/ubrn_bindgen/src/bindings/react_native/mod.rs index 0fa7a8c..6722866 100644 --- a/crates/ubrn_bindgen/src/bindings/react_native/mod.rs +++ b/crates/ubrn_bindgen/src/bindings/react_native/mod.rs @@ -18,7 +18,7 @@ use ubrn_common::{fmt, run_cmd_quietly}; use uniffi_bindgen::{ interface::{ FfiArgument, FfiCallbackFunction, FfiDefinition, FfiField, FfiFunction, FfiStruct, FfiType, - Function, + Function, Method, Object, }, BindingGenerator, BindingsConfig, ComponentInterface, }; @@ -206,6 +206,7 @@ impl ComponentInterface { self.iter_ffi_functions_js_to_cpp_and_back() .chain(self.iter_ffi_functions_js_to_rust()) .chain(self.iter_ffi_functions_init_callback()) + .chain(self.iter_ffi_function_bless_pointer()) } fn iter_ffi_functions_js_to_rust(&self) -> impl Iterator { @@ -235,6 +236,12 @@ impl ComponentInterface { })) } + fn iter_ffi_function_bless_pointer(&self) -> impl Iterator { + self.object_definitions() + .iter() + .map(|o| o.ffi_function_bless_pointer()) + } + fn iter_ffi_structs(&self) -> impl Iterator { self.ffi_definitions().filter_map(|s| match s { FfiDefinition::Struct(s) => Some(s), @@ -351,6 +358,31 @@ fn store_with_name(types: &mut HashMap, type_: &Type) -> String { name } +#[ext] +impl Object { + fn ffi_function_bless_pointer(&self) -> FfiFunction { + let meta = uniffi_meta::MethodMetadata { + module_path: "internal".to_string(), + self_name: self.name().to_string(), + name: "ffi__bless_pointer".to_owned(), + is_async: false, + inputs: Default::default(), + return_type: None, + throws: None, + checksum: None, + docstring: None, + takes_self_by_arc: false, + }; + let func: Method = meta.into(); + let mut ffi = func.ffi_func().clone(); + ffi.init( + Some(FfiType::RustArcPtr(String::from(""))), + vec![FfiArgument::new("pointer", FfiType::UInt64)], + ); + ffi + } +} + #[ext] impl FfiFunction { fn is_internal(&self) -> bool { diff --git a/fixtures/callbacks/tests/bindings/test_callbacks.ts b/fixtures/callbacks/tests/bindings/test_callbacks.ts index cd60c75..1d60df2 100644 --- a/fixtures/callbacks/tests/bindings/test_callbacks.ts +++ b/fixtures/callbacks/tests/bindings/test_callbacks.ts @@ -170,6 +170,7 @@ class StoredTypeScriptStringifier implements StoredForeignStringifier { } } } + test("A callback passed into the constructor", (t) => { const stringifierCallback = new StoredTypeScriptStringifier(); const rustStringifier = new RustStringifier(stringifierCallback); @@ -178,4 +179,15 @@ test("A callback passed into the constructor", (t) => { const observed = rustStringifier.fromSimpleType(42); t.assertEqual(observed, expected); + + rustStringifier.uniffiDestroy(); +}); + +test("A callback passed into the constructor is not destroyed", (t) => { + const stringifierCallback = new StoredTypeScriptStringifier(); + const rustStringifier = new RustStringifier(stringifierCallback); + + // The destructor calls into the Rust, which calls back into Javascript. + // If the jsi::Runtime has been destroyed already, then this will cause a + // crash at the end of the run. This should be prevented. }); diff --git a/fixtures/coverall/tests/bindings/test_coverall.ts b/fixtures/coverall/tests/bindings/test_coverall.ts index afee78a..d96cf4b 100644 --- a/fixtures/coverall/tests/bindings/test_coverall.ts +++ b/fixtures/coverall/tests/bindings/test_coverall.ts @@ -90,6 +90,26 @@ test("test create_none_dict() with default values", (t) => { t.assertEqual(d.coveralls, undefined); }); +test("Given a rust object, when it is destroyed, it cannot be re-used", (t) => { + const name = "To be destroyed"; + const c = new Coveralls(name); + + // check it works first. + t.assertEqual(name, c.getName()); + + // then destroy it. + c.uniffiDestroy(); + + // now check it doesn't work. + t.assertThrows( + () => true, + () => c.getName(), + ); + + // …destroy again, just to show it's idempotent. + c.uniffiDestroy(); +}); + test("Given 1000 objects, when they go out of scope, then they are dropped by rust", (t) => { // The GC test; we should have 1000 alive by the end of the loop. // @@ -249,3 +269,9 @@ test("Dummy coveralls implement the Coveralls interface", (t) => { const extendedCoveralls = new ExtendedCoveralls("Extended coveralls"); t.assertTrue(Coveralls.instanceOf(extendedCoveralls)); }); + +// This is the last test, so we can test a graceful shutdown. +test("Given a rust object, if not destroyed, it's ok", (t) => { + const name = "To be destroyed by GC"; + const c = new Coveralls(name); +}); diff --git a/typescript/src/rust-call.ts b/typescript/src/rust-call.ts index 438b4a4..3e9a632 100644 --- a/typescript/src/rust-call.ts +++ b/typescript/src/rust-call.ts @@ -98,8 +98,5 @@ function uniffiCheckCallStatus( export type UniffiRustArcPtrDestructor = (pointer: bigint) => void; export type UniffiRustArcPtr = { - // pointer - p: UnsafeMutableRawPointer; - // destructor - d: UniffiRustArcPtrDestructor; + markDestroyed(): void; };