From 909d1d058679aaab4eff5482f50e65fc0a552a36 Mon Sep 17 00:00:00 2001 From: James Hugman Date: Mon, 11 Nov 2024 14:31:00 +0000 Subject: [PATCH 1/2] Reproduce callback deadlock A minimal test which exhibits a deadlock. On my machine, I can get it to deadlock about 25-35% of the time. This is with the script: ```sh fixture=fixtures/callbacks-regression for i in {1..50} ; do echo echo "Starting $i" cargo xtask run \ --crate $fixture \ --ts-dir $fixture/generated \ --cpp-dir $fixture/generated/cpp \ $fixture/tests/bindings/test_callbacks_regression.ts done ``` Counting the incidence of `timed out` in the output and multiplying by 2. Increasing the work done in the callback (e.g. by uncommenting the `console.log`) gave this error. I'm fairly sure that this is a separate error, and likely an artifact of the test-harness. ```sh -- Going to 1024, now at: 264 -- Going to 1024, now at: 265 Assertion failed: (usedDbg_ && "Stats not submitted."), function ~CollectionStats, file HadesGC.cpp, line 259. ``` This error is the reason why I can't make a test case to reproduce on CI. For now, I'm going to treat this error as different to the deadlock. --- fixtures/callbacks-regression/Cargo.toml | 20 +++++++ fixtures/callbacks-regression/README.md | 5 ++ fixtures/callbacks-regression/build.rs | 7 +++ fixtures/callbacks-regression/src/lib.rs | 35 ++++++++++++ .../bindings/test_callbacks_regression.ts | 53 +++++++++++++++++++ .../tests/test_generated_bindings.rs | 5 ++ fixtures/callbacks-regression/uniffi.toml | 0 7 files changed, 125 insertions(+) create mode 100644 fixtures/callbacks-regression/Cargo.toml create mode 100644 fixtures/callbacks-regression/README.md create mode 100644 fixtures/callbacks-regression/build.rs create mode 100644 fixtures/callbacks-regression/src/lib.rs create mode 100644 fixtures/callbacks-regression/tests/bindings/test_callbacks_regression.ts create mode 100644 fixtures/callbacks-regression/tests/test_generated_bindings.rs create mode 100644 fixtures/callbacks-regression/uniffi.toml diff --git a/fixtures/callbacks-regression/Cargo.toml b/fixtures/callbacks-regression/Cargo.toml new file mode 100644 index 00000000..0839c845 --- /dev/null +++ b/fixtures/callbacks-regression/Cargo.toml @@ -0,0 +1,20 @@ +[package] +name = "uniffi-example-callbacks" +edition = "2021" +version = "0.22.0" +license = "MPL-2.0" +publish = false + +[lib] +crate-type = ["lib", "cdylib"] +name = "uniffi_callbacks" + +[dependencies] +uniffi = { workspace = true } +thiserror = "1.0" + +[build-dependencies] +uniffi = { workspace = true, features = ["build"] } + +[dev-dependencies] +uniffi = { workspace = true, features = ["bindgen-tests"] } diff --git a/fixtures/callbacks-regression/README.md b/fixtures/callbacks-regression/README.md new file mode 100644 index 00000000..4c57c1d5 --- /dev/null +++ b/fixtures/callbacks-regression/README.md @@ -0,0 +1,5 @@ +This is an example of UniFFI "callbacks". + +Callbacks are the ability to implement Rust traits in foreign languages. These are defined by +[normal UniFFI traits](../../docs/manual/src/foreign_traits.md) or via ["callback" definitions](../../docs/manual/src/udl/callback_interfaces.md). + diff --git a/fixtures/callbacks-regression/build.rs b/fixtures/callbacks-regression/build.rs new file mode 100644 index 00000000..1ef3bb78 --- /dev/null +++ b/fixtures/callbacks-regression/build.rs @@ -0,0 +1,7 @@ +/* + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/ + */ + +fn main() {} diff --git a/fixtures/callbacks-regression/src/lib.rs b/fixtures/callbacks-regression/src/lib.rs new file mode 100644 index 00000000..e2e323e0 --- /dev/null +++ b/fixtures/callbacks-regression/src/lib.rs @@ -0,0 +1,35 @@ +/* + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/ + */ + +use std::{sync::Arc, thread::spawn}; + +uniffi::setup_scaffolding!(); + +#[uniffi::export(with_foreign)] +pub trait EventListener: Send + Sync { + fn on_event(&self, message: String, number: i32); +} + +#[derive(uniffi::Object)] +struct EventSource { + listener: Arc, +} + +#[uniffi::export] +impl EventSource { + #[uniffi::constructor] + fn new(listener: Arc) -> Arc { + Arc::new(Self { listener }) + } + fn start(self: Arc, message: String, until: i32) { + let listener = self.listener.clone(); + spawn(move || { + for i in 0..until { + listener.on_event(message.clone(), i); + } + }); + } +} diff --git a/fixtures/callbacks-regression/tests/bindings/test_callbacks_regression.ts b/fixtures/callbacks-regression/tests/bindings/test_callbacks_regression.ts new file mode 100644 index 00000000..30ead8df --- /dev/null +++ b/fixtures/callbacks-regression/tests/bindings/test_callbacks_regression.ts @@ -0,0 +1,53 @@ +/* + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/ + */ + +/* +fixture=fixtures/callbacks-regression +cargo xtask run \ + --crate $fixture \ + --ts-dir $fixture/generated \ + --cpp-dir $fixture/generated/cpp \ + $fixture/tests/bindings/test_callbacks_regression.ts +*/ + +import generated, { + EventSource, + EventListener, +} from "../../generated/uniffi_callbacks"; +import { asyncTest, AsyncAsserts } from "@/asserts"; +import { console } from "@/hermes"; + +// This is only needed in tests. +generated.initialize(); + +class EventListenerImpl implements EventListener { + constructor( + private t: AsyncAsserts, + private max: number, + ) {} + onEvent(message: string, number: number): void { + // console.log("--", message, number); + if (number === this.max - 1) { + console.log("-- Done!", this.max); + this.t.end(); + } + } +} + +async function testToMax(max: number, t: AsyncAsserts) { + const listener = new EventListenerImpl(t, max); + const source = new EventSource(listener); + source.start(`Going to ${max}, now at:`, max); +} + +(async () => { + for (let i = 1; i <= 1024; i *= 2) { + await asyncTest( + `Full tilt test up to ${i}`, + async (t) => await testToMax(i, t), + ); + } +})(); diff --git a/fixtures/callbacks-regression/tests/test_generated_bindings.rs b/fixtures/callbacks-regression/tests/test_generated_bindings.rs new file mode 100644 index 00000000..24ffd28d --- /dev/null +++ b/fixtures/callbacks-regression/tests/test_generated_bindings.rs @@ -0,0 +1,5 @@ +/* + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/ + */ diff --git a/fixtures/callbacks-regression/uniffi.toml b/fixtures/callbacks-regression/uniffi.toml new file mode 100644 index 00000000..e69de29b From 7f61add4b6311141570169e39bc3b547159ae6a4 Mon Sep 17 00:00:00 2001 From: James Hugman Date: Mon, 11 Nov 2024 14:31:22 +0000 Subject: [PATCH 2/2] Fix the callbacks deadlock The change here is in `UniffiCallInvoker`, reverting the implementation of `invokeBlocking` back to the Promise based implementation introduced in PR #88. The deadlock rate for the test is now 0/50. Adding any more to the implementation (e.g. uncommenting the `console.log`) causes the same error as before, but it is completely deterministic: ```sh -- Going to 1024, now at: 264 -- Going to 1024, now at: 265 Assertion failed: (usedDbg_ && "Stats not submitted."), function ~CollectionStats, file HadesGC.cpp, line 259. ``` On my machine it _always_ stops after 265/1024. I am still unsure if this is a problem with production uniff-bindgen-react-native code, an artifact of the test-harness or something weirder. I'm going to suggest that this is addressed in a separate issue. --- cpp/includes/UniffiCallInvoker.h | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/cpp/includes/UniffiCallInvoker.h b/cpp/includes/UniffiCallInvoker.h index 520f462d..bdfaf407 100644 --- a/cpp/includes/UniffiCallInvoker.h +++ b/cpp/includes/UniffiCallInvoker.h @@ -6,6 +6,7 @@ #pragma once #include #include +#include #include #include #include @@ -58,26 +59,19 @@ class UniffiCallInvoker { if (std::this_thread::get_id() == threadId_) { func(rt); } else { - std::mutex mtx; - std::condition_variable cv; - bool done = false; + std::promise promise; + auto future = promise.get_future(); // The runtime argument was added to CallFunc in // https://github.com/facebook/react-native/pull/43375 // // 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]() { + // react::CallFunc wrapper = [&func, &promise](jsi::Runtime &rt) { + std::function wrapper = [&func, &promise, &rt]() { func(rt); - { - std::lock_guard lock(mtx); - done = true; - } - cv.notify_one(); + promise.set_value(); }; callInvoker_->invokeAsync(std::move(wrapper)); - - std::unique_lock lock(mtx); - cv.wait(lock, [&done] { return done; }); + future.wait(); } }