From f657971eac7ba0fb5aad8fb71fdd539473742877 Mon Sep 17 00:00:00 2001 From: James Hugman Date: Thu, 5 Sep 2024 16:30:34 +0100 Subject: [PATCH] Fixup deadlock when hammering futures of a certain type --- fixtures/async-deadlock/Cargo.toml | 28 +++++ fixtures/async-deadlock/src/lib.rs | 16 +++ .../tests/bindings/test_async_deadlock.ts | 107 ++++++++++++++++++ fixtures/async-deadlock/uniffi.toml | 3 + typescript/src/async-rust-call.ts | 27 ++++- 5 files changed, 179 insertions(+), 2 deletions(-) create mode 100644 fixtures/async-deadlock/Cargo.toml create mode 100644 fixtures/async-deadlock/src/lib.rs create mode 100644 fixtures/async-deadlock/tests/bindings/test_async_deadlock.ts create mode 100644 fixtures/async-deadlock/uniffi.toml diff --git a/fixtures/async-deadlock/Cargo.toml b/fixtures/async-deadlock/Cargo.toml new file mode 100644 index 00000000..37f7e9c8 --- /dev/null +++ b/fixtures/async-deadlock/Cargo.toml @@ -0,0 +1,28 @@ +[package] +name = "regression-async-deadlock" +edition = "2021" +version = "0.0.1" +authors = ["Filament Team "] +license = "MPL-2.0" +publish = false + +[lib] +crate-type = ["lib", "staticlib", "cdylib"] +name = "regression_async_deadlock" + +[patch.crates-io] +async-compat = { git = "https://github.com/jplatte/async-compat", rev = "16dc8597ec09a6102d58d4e7b67714a35dd0ecb8" } +const_panic = { git = "https://github.com/jplatte/const_panic", rev = "9024a4cb3eac45c1d2d980f17aaee287b17be498" } + +[dependencies] +uniffi = { workspace = true } +matrix-sdk-ffi = { workspace = true } +matrix-sdk = { workspace = true } +thiserror = "1.0" + + +[build-dependencies] +uniffi = { workspace = true, features = ["build"] } + +[dev-dependencies] +uniffi = { workspace = true, features = ["bindgen-tests"] } diff --git a/fixtures/async-deadlock/src/lib.rs b/fixtures/async-deadlock/src/lib.rs new file mode 100644 index 00000000..7cf36194 --- /dev/null +++ b/fixtures/async-deadlock/src/lib.rs @@ -0,0 +1,16 @@ +/* + * 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; + +use matrix_sdk_ffi::client_builder::ClientBuilder; + +#[uniffi::export] +pub fn get_matrix_client_builder() -> Arc { + ClientBuilder::new() +} + +uniffi::setup_scaffolding!(); diff --git a/fixtures/async-deadlock/tests/bindings/test_async_deadlock.ts b/fixtures/async-deadlock/tests/bindings/test_async_deadlock.ts new file mode 100644 index 00000000..5953d03f --- /dev/null +++ b/fixtures/async-deadlock/tests/bindings/test_async_deadlock.ts @@ -0,0 +1,107 @@ +/* + * 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/ + */ + +import { asyncTest } from "@/asserts"; +import { console } from "@/hermes"; +import { + ClientBuilder, + ClientInterface, + mediaSourceFromUrl, + MediaSourceInterface, +} from "../../generated/matrix_sdk_ffi"; + +import { delayPromise } from "uniffi-bindgen-react-native"; + +const url = "mxc://matrix.1badday.com/RUUIKNjovSSwYbULWuNQarDA"; + +(async () => { + await asyncTest("Test delay promise", async (t) => { + console.info("Starting delay"); + await delayPromise(0); + const start = Date.now(); + await delayPromise(1000); + const actual = Date.now() - start; + console.info(`Ending delay, measured: ${actual} ms`); + t.assertInRange(actual, 900, 1100); + + t.end(); + }); + + await asyncTest("Test for deadlock 8", async (t) => { + await loadImages(8); + t.end(); + }); + + await asyncTest("Test for deadlock 16", async (t) => { + await loadImages(16); + t.end(); + }); + + + await asyncTest("Test for deadlock 32", async (t) => { + await loadImages(32); + t.end(); + }); + + await asyncTest("Test for deadlock 64", async (t) => { + await loadImages(64); + t.end(); + }); + + await asyncTest("Test for deadlock 128", async (t) => { + await loadImages(128); + t.end(); + }); + + await asyncTest("Test for deadlock 256", async (t) => { + await loadImages(256); + t.end(); + }); + + await asyncTest("Test for deadlock 512", async (t) => { + await loadImages(512); + t.end(); + }, 30 * 1000); + + await asyncTest("Test for deadlock 1024", async (t) => { + await loadImages(1024); + t.end(); + }, 60 * 1000); + +})(); + +async function loadImages(n: number): Promise { + const images = new Array(n).fill(url); + const sourcedImages = images.map((i) => ({ + source: mediaSourceFromUrl(i), + })); + const client = await new ClientBuilder() + .homeserverUrl("https://matrix.1badday.com/") + .build(); + console.log("Starting…"); + const promises = sourcedImages.map((s, i) => { + console.log(`-- About to load image ${i}/${n}: ${s.source.url()}`); + return client.getMediaContent(s.source).then((content) => { + console.log(`-- Loaded image ${i}/${n}`, s.source.url()); + }); + }); + await Promise.allSettled(promises); + console.log("… finished"); +} + +// (async function () { +// const client = await new ClientBuilder() +// .homeserverUrl("https://matrix.1badday.com/") +// .build(); +// const promises = sourcedImages.map((s) => { +// console.log(`-- About to load ${s.source.url}`); +// client.getMediaContent(s.source).then((content) => { +// console.log("-- Loaded image: ", s.source.url()); +// }); +// }); + +// await Promise.allSettled(promises); +// })(); diff --git a/fixtures/async-deadlock/uniffi.toml b/fixtures/async-deadlock/uniffi.toml new file mode 100644 index 00000000..855344af --- /dev/null +++ b/fixtures/async-deadlock/uniffi.toml @@ -0,0 +1,3 @@ +[bindings.typescript] +isVerbose = true +consoleImport = "@/hermes" diff --git a/typescript/src/async-rust-call.ts b/typescript/src/async-rust-call.ts index df0e0e21..52f509ba 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. * @@ -65,13 +71,30 @@ export async function uniffiRustCallAsync( // The poll, complete and free methods are specialized by the FFIType of the return value. try { let pollResult: number | undefined; - do { + while (true) { // Calling pollFunc with a callback that resolves the promise that pollRust // returns: pollRust makes the promise, uniffiFutureContinuationCallback resolves it. pollResult = await pollRust((handle) => { pollFunc(rustFuture, uniffiFutureContinuationCallback, handle); }); - } while (pollResult != UNIFFI_RUST_FUTURE_POLL_READY); + // 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 + if (pollResult == UNIFFI_RUST_FUTURE_POLL_READY) { + break; + } + await delayPromise(100); + } // Now it's ready, all we need to do is pick up the result (and error). return liftFunc(