Keep Zano calls off the RN bridge thread - #20
Conversation
On the legacy Android architecture every @ReactMethod runs on the one shared mqt_native_modules thread, and callZano ran its JNI synchronously there. That thread also executes UIManager's view commands, so any Zano call that blocked in C++ froze every view update in the app: buttons dead, sync banner frozen, only UI-thread-driven scrolling and gestures alive. QA hit exactly this about seven minutes after login, when the first checkpoint's close_wallet -- which holds the manager's global wallets lock exclusively while it waits out the refresh worker -- kept the bridge thread captive (and, on Android's writer-preferring rwlock, deadlocked permanently; see the companion accountbased change). Reproduced on an API 35 emulator with checkpoints tortured to 30s: a mid-freeze JDWP dump shows mqt_native_modules parked inside RnZanoModule.callZanoJNI. With this change the same scenario shows the blocked call quarantined on the executor thread while mqt_native_modules, mqt_js and main all stay idle and rendering continues. A single-thread executor preserves the strict global call ordering the shared bridge thread provided; the arguments are copied out of the ReadableArray on the caller thread before the hop, and promise resolution is thread-safe. iOS is untouched -- its TurboModule queue never ran UIManager, which is why identical engine code never froze rendering there.
j0ntz
left a comment
j0ntz
left a comment
There was a problem hiding this comment.
Neither of these blocks the PR. RnMoneroModule in react-native-monero-lwsf uses the identical executor pattern, so both apply there too.
| // scan chunk, or the SDK's close-during-scan lock inversion) froze every | ||
| // view update in the app. A single thread preserves the strict global | ||
| // call ordering the shared bridge thread provided. | ||
| private final ExecutorService executor = Executors.newSingleThreadExecutor(); |
There was a problem hiding this comment.
Warning: scoping the executor to the module instance narrows the ordering guarantee the comment above claims.
mqt_native_modules is one thread per process, so its ordering held across React context recreations. An instance field does not. A JS reload builds a new RnZanoModule with a second executor while the first one's thread is still alive and possibly still draining queued calls, so two threads then call into the same process-global Zano SDK. That is the concurrent close-during-scan shape this PR exists to contain.
sequenceDiagram
participant JS
participant Exec1 as executor, instance 1
participant Exec2 as executor, instance 2
participant SDK as Zano SDK, process-global
JS->>Exec1: closeWallet(w)
Note over Exec1,SDK: blocked in C++
JS->>JS: reload rebuilds the ReactContext
JS->>Exec2: open(w)
Exec2->>SDK: open
Exec1->>SDK: closeWallet resumes
Note over SDK: two threads, ordering gone
private static final ExecutorService executor restores the process-global property and drops the per-reload thread leak in the same line. An invalidate() override calling shutdown() fixes only the leak: the in-flight call still races the new instance.
Same declaration, separate nit: Executors.newSingleThreadExecutor(r -> new Thread(r, "zano")) names the thread. The evidence in this PR's description is a thread dump, and pool-1-thread-1 will not identify itself in the next one.
| executor.execute(() -> { | ||
| try { | ||
| promise.resolve(callZanoJNI(method, strings)); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Suggestion: catch (Exception e) no longer covers everything the promise depends on.
The clause is unchanged, but the thread under it is not. On mqt_native_modules an escaping Error reached RN's NativeModuleCallExceptionHandler (redbox in dev, an RN-owned crash in prod). Out of an executor.execute Runnable it goes to the default uncaught handler instead, which kills the process. promise was never settled in that path before either; catch (Throwable e) costs one word and gets the JS caller a ZanoError instead of silence.
2 participants
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
none — companion to EdgeApp/edge-currency-accountbased#1094, but each lands
independently. This PR contains the app-wide blast radius; #1094 removes the
deadlock trigger on Android.
Description
Fixes the Android freeze QA reported: about seven minutes after login, taps and
buttons die while scrolling and the drawer keep working, and the Zano sync
banner stops updating. iOS is unaffected.
Mechanism. On the legacy Android architecture every
@ReactMethodruns onthe single shared
mqt_native_modulesthread, andcallZanoran its JNIsynchronously there. That thread also executes UIManager's view commands, so
any Zano call that blocks in C++ freezes every view update in the app.
The blocker QA hit is the checkpoint's
close_wallet, which holds themanager's global wallets lock exclusively while waiting out the refresh
worker — and on Android's writer-preferring rwlock that inverts into a
permanent deadlock with the worker's own shared-lock acquisition
(
wallets_manager.cppdocuments the ordering hazard at line 2159). Oneblocked sync call then held the bridge thread captive indefinitely: buttons
dead (UIManager), banner frozen (every native call queued, including the
tryPullResultpolls that would have observed the close finishing), scrolland drawer alive (pure UI thread). QA's screen recording matches this
signature exactly — the sync banner reads the same block count across four
minutes.
Repro evidence (API 35 emulator, checkpoints tortured to a 30s interval,
4 wallets in deep catch-up): a JDWP thread dump captured mid-freeze shows
with checkpoint writes stalled for minutes.
The change.
callZanohops to a dedicated single-thread executor andresolves the promise from there. Argument extraction from the
ReadableArraystays on the caller thread; a single thread preserves the strict global call
ordering the shared bridge thread provided. Re-running the identical torture
scenario with this change, the blocked call sits quarantined on the executor
thread while
mqt_native_modules,mqt_js, andmainall stay idle andrendering continues.
Scope. This contains the blast radius — a blocked Zano call stalls only
Zano. It deliberately does not fix the deadlock itself: that trigger is
removed on Android by EdgeApp/edge-currency-accountbased#1094, and
eliminated for real by an SDK fix to
close_wallet's lock ordering (upstreamask to follow). iOS behavior is unchanged — its TurboModule queue never ran
UIManager, which is why identical engine code never froze rendering there.