From 404b003df2fcc9724b1a65ea2760f2feed0ecb23 Mon Sep 17 00:00:00 2001 From: jhugman Date: Tue, 17 Sep 2024 18:53:14 +0100 Subject: [PATCH] Fixup deadlock when hammering futures of a certain type (#88) According to [The Big O of Code Reviews](https://www.egorand.dev/the-big-o-of-code-reviews/), this is a O(_n_) change. This PR moves the polling of futures to outside of the continuation callback; this has shown to be a source of deadlocks. ``` // 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 ``` Unfortunately, the tests for this aren't likely to end up in this repo, since it relies on a fork of the matrix-sdk. As said in the comment, I'm don't think this has fixed all possible deadlocks, but I'm _hoping_ this has at least fixed the proximal problem. Fixes #89 --- cpp/includes/UniffiCallInvoker.h | 38 +++++++++++++------ .../gen_cpp/templates/CallbackFunction.cpp | 4 ++ .../src/bindings/react_native/mod.rs | 4 ++ typescript/src/async-rust-call.ts | 9 ++++- 4 files changed, 42 insertions(+), 13 deletions(-) diff --git a/cpp/includes/UniffiCallInvoker.h b/cpp/includes/UniffiCallInvoker.h index 598305c5..520f462d 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,24 +58,37 @@ 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; }); } } + + /** + * Invokes the given function on the JS thread, by adding to + * the event queue. + */ + void invokeNonBlocking(jsi::Runtime &rt, UniffiCallFunc func) { + // react::CallFunc wrapper = [func](jsi::Runtime &rt) { + 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..385ad427 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 @@ -187,7 +187,11 @@ namespace {{ ns }} { }; // We'll then call that lambda from the callInvoker which will // look after calling it on the correct thread. + {% if callback.is_blocking() -%} callInvoker->invokeBlocking(rt, jsLambda); + {%- else %} + callInvoker->invokeNonBlocking(rt, jsLambda); + {%- endif %} }; return callback; } diff --git a/crates/ubrn_bindgen/src/bindings/react_native/mod.rs b/crates/ubrn_bindgen/src/bindings/react_native/mod.rs index cce42976..1f12d6e8 100644 --- a/crates/ubrn_bindgen/src/bindings/react_native/mod.rs +++ b/crates/ubrn_bindgen/src/bindings/react_native/mod.rs @@ -501,6 +501,10 @@ impl FfiCallbackFunction { .find(|a| a.is_return() && !a.type_().is_void()); arg.map(|a| a.type_()) } + + fn is_blocking(&self) -> bool { + self.name() != "RustFutureContinuationCallback" + } } fn is_future(nm: &str) -> bool { diff --git a/typescript/src/async-rust-call.ts b/typescript/src/async-rust-call.ts index c7a7b59c..f37db17b 100644 --- a/typescript/src/async-rust-call.ts +++ b/typescript/src/async-rust-call.ts @@ -92,7 +92,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( @@ -128,6 +128,13 @@ const uniffiFutureContinuationCallback: UniffiRustFutureContinuationCallback = ( pollResult: number, ) => { const resolve = UNIFFI_RUST_FUTURE_RESOLVER_MAP.remove(handle); + // 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. + // + // We avoid this by using UniffiCallInvoker::invokeNonBlocking for this callback. resolve(pollResult); };