From 3a6521d3a143f24f64945fe5815f9e182668d942 Mon Sep 17 00:00:00 2001 From: Wes Chow Date: Thu, 25 Jul 2024 13:40:05 -0400 Subject: [PATCH 1/5] Simplify React name trimming function. --- crates/ubrn_cli/src/config/mod.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/crates/ubrn_cli/src/config/mod.rs b/crates/ubrn_cli/src/config/mod.rs index 9e5806f6..5172ec72 100644 --- a/crates/ubrn_cli/src/config/mod.rs +++ b/crates/ubrn_cli/src/config/mod.rs @@ -56,11 +56,7 @@ impl ProjectConfig { } fn trim_react_native(name: &str) -> String { - name.strip_prefix("RN") - .unwrap_or(name) - .replace("ReactNative", "") - .replace("react-native", "") - .trim_matches('-') + name.trim_matches('-') .trim_matches('_') .to_string() } @@ -181,7 +177,7 @@ impl TurboModulesConfig { fn default_spec_name() -> String { let package_json = workspace::package_json(); let codegen_name = &package_json.codegen().name; - format!("Native{}", trim_react_native(codegen_name)) + trim_react_native(codegen_name) } } From db5d739cf5ecae7f04c9845bd7db2ab580fca338 Mon Sep 17 00:00:00 2001 From: Daniel Salinas Date: Tue, 10 Sep 2024 13:00:45 -0400 Subject: [PATCH 2/5] Mark fields as non private --- .../react_native/gen_typescript/templates/ErrorTemplate.ts | 4 ++-- .../gen_typescript/templates/TaggedEnumTemplate.ts | 2 +- typescript/tests/playground/enums.ts | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/templates/ErrorTemplate.ts b/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/templates/ErrorTemplate.ts index 25ff5819..6648ab7a 100644 --- a/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/templates/ErrorTemplate.ts +++ b/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/templates/ErrorTemplate.ts @@ -9,8 +9,8 @@ export const {{ decl_type_name }} = (() => { {%- call ts::docstring(variant, 4) %} {%- let variant_name = variant.name()|class_name(ci) %} class {{ variant_name }} extends UniffiError { - private readonly __uniffiTypeName = "{{ type_name }}"; - private readonly __variant = {{ loop.index }}; + readonly __uniffiTypeName = "{{ type_name }}"; + readonly __variant = {{ loop.index }}; constructor(message: string) { super("{{ type_name }}", "{{ variant_name }}", message); } diff --git a/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/templates/TaggedEnumTemplate.ts b/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/templates/TaggedEnumTemplate.ts index 3bda8580..ff39e65c 100644 --- a/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/templates/TaggedEnumTemplate.ts +++ b/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/templates/TaggedEnumTemplate.ts @@ -56,7 +56,7 @@ export const {{ decl_type_name }} = (() => { {% call ts::docstring(variant, 4) %} class {{ variant_name }} extends {{ superclass }} implements {{ variant_interface }} { - private readonly __uniffiTypeName = "{{ type_name }}"; + readonly __uniffiTypeName = "{{ type_name }}"; readonly tag = {{ variant_tag }}; {%- if has_fields %} readonly inner: {% call variant_data_type(variant) %}; diff --git a/typescript/tests/playground/enums.ts b/typescript/tests/playground/enums.ts index 38c068d9..9092d1c3 100644 --- a/typescript/tests/playground/enums.ts +++ b/typescript/tests/playground/enums.ts @@ -12,7 +12,7 @@ export const MyEnum = (() => { inner: Readonly<{ myValue: string }>; }; class Variant1_ extends UniffiEnum implements Variant1__interface { - private readonly __uniffiTypeName = typeName; + readonly __uniffiTypeName = typeName; readonly tag = "Variant1"; readonly inner: Readonly<{ myValue: string }>; constructor(inner: { myValue: string }) { @@ -29,7 +29,7 @@ export const MyEnum = (() => { inner: Readonly<[number, string]>; }; class Variant2_ extends UniffiEnum implements Variant2__interface { - private readonly __uniffiTypeName = typeName; + readonly __uniffiTypeName = typeName; readonly tag = "Variant2"; readonly inner: Readonly<[number, string]>; constructor(p1: number, p2: string) { From 2dd764ab0ddf6647d88904ab953e85256cb6642b Mon Sep 17 00:00:00 2001 From: Daniel Salinas Date: Tue, 10 Sep 2024 13:32:37 -0400 Subject: [PATCH 3/5] Add private field comments --- .../gen_typescript/templates/ErrorTemplate.ts | 8 ++++++++ .../gen_typescript/templates/TaggedEnumTemplate.ts | 4 ++++ 2 files changed, 12 insertions(+) diff --git a/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/templates/ErrorTemplate.ts b/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/templates/ErrorTemplate.ts index 6648ab7a..ba79e2c6 100644 --- a/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/templates/ErrorTemplate.ts +++ b/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/templates/ErrorTemplate.ts @@ -9,7 +9,15 @@ export const {{ decl_type_name }} = (() => { {%- call ts::docstring(variant, 4) %} {%- let variant_name = variant.name()|class_name(ci) %} class {{ variant_name }} extends UniffiError { + /** + * @private + * This field is private and should not be used. + */ readonly __uniffiTypeName = "{{ type_name }}"; + /** + * @private + * This field is private and should not be used. + */ readonly __variant = {{ loop.index }}; constructor(message: string) { super("{{ type_name }}", "{{ variant_name }}", message); diff --git a/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/templates/TaggedEnumTemplate.ts b/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/templates/TaggedEnumTemplate.ts index ff39e65c..789a5400 100644 --- a/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/templates/TaggedEnumTemplate.ts +++ b/crates/ubrn_bindgen/src/bindings/react_native/gen_typescript/templates/TaggedEnumTemplate.ts @@ -56,6 +56,10 @@ export const {{ decl_type_name }} = (() => { {% call ts::docstring(variant, 4) %} class {{ variant_name }} extends {{ superclass }} implements {{ variant_interface }} { + /** + * @private + * This field is private and should not be used, use `tag` instead. + */ readonly __uniffiTypeName = "{{ type_name }}"; readonly tag = {{ variant_tag }}; {%- if has_fields %} From 9bae6225e57682203dd52223813c29768238b0eb Mon Sep 17 00:00:00 2001 From: Daniel Salinas Date: Thu, 12 Sep 2024 15:18:23 -0400 Subject: [PATCH 4/5] cherry-pick of possible deadlock fix --- cpp/includes/UniffiCallInvoker.h | 28 ++++++++++++++++------------ typescript/src/async-rust-call.ts | 29 ++++++++++++++++++++++++++--- 2 files changed, 42 insertions(+), 15 deletions(-) diff --git a/cpp/includes/UniffiCallInvoker.h b/cpp/includes/UniffiCallInvoker.h index 598305c5..90ddd424 100644 --- a/cpp/includes/UniffiCallInvoker.h +++ b/cpp/includes/UniffiCallInvoker.h @@ -5,9 +5,10 @@ */ #pragma once #include -#include +#include #include #include +#include #include namespace uniffi_runtime { @@ -57,23 +58,26 @@ class UniffiCallInvoker { if (std::this_thread::get_id() == threadId_) { func(rt); } else { - std::promise promise; - auto future = promise.get_future(); + std::mutex mtx; + std::condition_variable cv; + bool done = false; // The runtime argument was added to CallFunc in // https://github.com/facebook/react-native/pull/43375 // - // Once that is released, there will be a deprecation period. - // - // Any time during the deprecation period, we can switch `&rt` - // from being a captured variable to being an argument, i.e. - // commenting out one line, and uncommenting the other. - std::function wrapper = [&func, &promise, &rt]() { - // react::CallFunc wrapper = [&func, &promise](jsi::Runtime &rt) { + // This can be changed once that change is released. + // react::CallFunc wrapper = [&func, &mtx, &cv, &done](jsi::Runtime &rt) { + std::function wrapper = [&func, &rt, &mtx, &cv, &done]() { func(rt); - promise.set_value(); + { + std::lock_guard lock(mtx); + done = true; + } + cv.notify_one(); }; callInvoker_->invokeAsync(std::move(wrapper)); - future.wait(); + + std::unique_lock lock(mtx); + cv.wait(lock, [&done] { return done; }); } } }; diff --git a/typescript/src/async-rust-call.ts b/typescript/src/async-rust-call.ts index df0e0e21..8d284a0b 100644 --- a/typescript/src/async-rust-call.ts +++ b/typescript/src/async-rust-call.ts @@ -33,6 +33,12 @@ type PollFunc = ( handle: UniffiHandle, ) => void; +// Calls setTimeout and then resolves the promise. +// This may be used as a simple yield. +export async function delayPromise(delayMs: number): Promise { + return new Promise((resolve) => setTimeout(resolve, delayMs)); +} + /** * This method calls an asynchronous method on the Rust side. * @@ -71,7 +77,7 @@ export async function uniffiRustCallAsync( pollResult = await pollRust((handle) => { pollFunc(rustFuture, uniffiFutureContinuationCallback, handle); }); - } while (pollResult != UNIFFI_RUST_FUTURE_POLL_READY); + } while (pollResult !== UNIFFI_RUST_FUTURE_POLL_READY); // Now it's ready, all we need to do is pick up the result (and error). return liftFunc( @@ -85,7 +91,7 @@ export async function uniffiRustCallAsync( // #RUST_TASK_CANCELLATION: the unused `cancelFunc` function should be exposed // to client code in order for clients to be able to cancel the running Rust task. } finally { - freeFunc(rustFuture); + setTimeout(() => freeFunc(rustFuture), 0); } } @@ -110,7 +116,24 @@ const uniffiFutureContinuationCallback: UniffiRustFutureContinuationCallback = ( pollResult: number, ) => { const resolve = UNIFFI_RUST_FUTURE_RESOLVER_MAP.remove(handle); - resolve(pollResult); + if (pollResult === UNIFFI_RUST_FUTURE_POLL_READY) { + resolve(pollResult); + } else { + // From https://github.com/mozilla/uniffi-rs/pull/1837/files#diff-8a28c9cf1245b4f714d406ea4044d68e1000099928eaca1afb504ccbc008fe9fR35-R37 + // + // > WARNING: the call to [rust_future_poll] must be scheduled to happen soon after the callback is + // > called, but not inside the callback itself. If [rust_future_poll] is called inside the + // > callback, some futures will deadlock and our scheduler code might as well. + // + // This delay is to ensure that `uniffiFutureContinuationCallback` returns before the next poll, i.e. + // so that the next poll is outside of this callback. + // + // The length of the delay seems to be significant (at least in tests which hammer a network). + // I would like to understand this more: I am still seeing deadlocks when this drops below its current + // delay, but these maybe related to a different issue, as alluded to in + // https://github.com/mozilla/uniffi-rs/pull/1901 + setTimeout(() => resolve(pollResult), 20); + } }; // For testing only. From de58bddef7835e5c93112dc003c61866edf45380 Mon Sep 17 00:00:00 2001 From: Daniel Salinas Date: Thu, 12 Sep 2024 15:45:35 -0400 Subject: [PATCH 5/5] Try again with async non-blockers --- cpp/includes/UniffiCallInvoker.h | 11 +++++++++++ .../gen_cpp/templates/CallbackFunction.cpp | 11 ++++++++--- .../react_native/gen_cpp/templates/Future.cpp | 3 ++- 3 files changed, 21 insertions(+), 4 deletions(-) diff --git a/cpp/includes/UniffiCallInvoker.h b/cpp/includes/UniffiCallInvoker.h index 90ddd424..8e27c2ba 100644 --- a/cpp/includes/UniffiCallInvoker.h +++ b/cpp/includes/UniffiCallInvoker.h @@ -80,5 +80,16 @@ class UniffiCallInvoker { cv.wait(lock, [&done] { return done; }); } } + + void invokeAsync(jsi::Runtime &rt, UniffiCallFunc &func) { + if (std::this_thread::get_id() == threadId_) { + func(rt); + } else { + std::function wrapper = [&func, &rt]() { + func(rt); + }; + callInvoker_->invokeAsync(std::move(wrapper)); + } + } }; } // namespace uniffi_runtime diff --git a/crates/ubrn_bindgen/src/bindings/react_native/gen_cpp/templates/CallbackFunction.cpp b/crates/ubrn_bindgen/src/bindings/react_native/gen_cpp/templates/CallbackFunction.cpp index 20aadbd2..156ff367 100644 --- a/crates/ubrn_bindgen/src/bindings/react_native/gen_cpp/templates/CallbackFunction.cpp +++ b/crates/ubrn_bindgen/src/bindings/react_native/gen_cpp/templates/CallbackFunction.cpp @@ -148,10 +148,11 @@ namespace {{ ns }} { makeCallbackFunction( // {{ ns }} jsi::Runtime &rt, std::shared_ptr callInvoker, - const jsi::Value &value) { + const jsi::Value &value, + bool invokeAsync = false) { auto callbackFunction = value.asObject(rt).asFunction(rt); auto callbackValue = std::make_shared(rt, callbackFunction); - rsLambda = [&rt, callInvoker, callbackValue]( + rsLambda = [&rt, callInvoker, callbackValue, invokeAsync]( {%- for arg in callback.arguments() %} {%- let arg_t = arg.type_().borrow()|ffi_type_name %} {%- let arg_nm_rs = arg.name()|var_name|fmt("rs_{}") %} @@ -187,7 +188,11 @@ namespace {{ ns }} { }; // We'll then call that lambda from the callInvoker which will // look after calling it on the correct thread. - callInvoker->invokeBlocking(rt, jsLambda); + if (invokeAsync) { + callInvoker->invokeAsync(rt, jsLambda); + } else { + callInvoker->invokeBlocking(rt, jsLambda); + } }; return callback; } diff --git a/crates/ubrn_bindgen/src/bindings/react_native/gen_cpp/templates/Future.cpp b/crates/ubrn_bindgen/src/bindings/react_native/gen_cpp/templates/Future.cpp index a7de0135..5980f643 100644 --- a/crates/ubrn_bindgen/src/bindings/react_native/gen_cpp/templates/Future.cpp +++ b/crates/ubrn_bindgen/src/bindings/react_native/gen_cpp/templates/Future.cpp @@ -14,7 +14,8 @@ template <> struct Bridging { static auto callback = {{ cb_type.borrow()|cpp_namespace(ci) }}::makeCallbackFunction( rt, callInvoker, - value + value, + true ); return callback; } catch (const std::logic_error &e) {