Skip to content

Commit

Permalink
Fixup deadlock when hammering futures of a certain type (#88)
Browse files Browse the repository at this point in the history
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
      // mozilla/uniffi-rs#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
  • Loading branch information
jhugman authored Sep 17, 2024
1 parent daec5f9 commit 404b003
Show file tree
Hide file tree
Showing 4 changed files with 42 additions and 13 deletions.
38 changes: 26 additions & 12 deletions cpp/includes/UniffiCallInvoker.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@
*/
#pragma once
#include <ReactCommon/CallInvoker.h>
#include <future>
#include <condition_variable>
#include <jsi/jsi.h>
#include <memory>
#include <mutex>
#include <thread>

namespace uniffi_runtime {
Expand Down Expand Up @@ -57,24 +58,37 @@ class UniffiCallInvoker {
if (std::this_thread::get_id() == threadId_) {
func(rt);
} else {
std::promise<void> 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<void()> 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<void()> wrapper = [&func, &rt, &mtx, &cv, &done]() {
func(rt);
promise.set_value();
{
std::lock_guard<std::mutex> lock(mtx);
done = true;
}
cv.notify_one();
};
callInvoker_->invokeAsync(std::move(wrapper));
future.wait();

std::unique_lock<std::mutex> 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<void()> wrapper = [func, &rt]() { func(rt); };
callInvoker_->invokeAsync(std::move(wrapper));
}
};
} // namespace uniffi_runtime
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
4 changes: 4 additions & 0 deletions crates/ubrn_bindgen/src/bindings/react_native/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
9 changes: 8 additions & 1 deletion typescript/src/async-rust-call.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ export async function uniffiRustCallAsync<F, T>(
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(
Expand Down Expand Up @@ -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);
};

Expand Down

0 comments on commit 404b003

Please sign in to comment.