From a319953c0967dd0d6c2a83a2b8a7b52d359a4d36 Mon Sep 17 00:00:00 2001 From: "lorenzo.padovani@padosoft.com" Date: Thu, 28 May 2026 23:37:59 +0200 Subject: [PATCH 1/2] fix(android): resolve ArrayBuffer via app class loader on worker threads Calling a command threw ClassNotFoundException for com.margelo.nitro.core.ArrayBuffer: the generated transport bridge looks up ArrayBuffer lazily in send() (JArrayBuffer::wrap -> FindClass) on a Nitro worker thread. A plain fbjni ThreadScope only attaches a JNIEnv; it does NOT install the app class loader, so FindClass hit the system loader (/system/lib64) and failed. The PR #8 fix only cached our transport's jclass on the JS thread and doesn't cover NitroModules core classes resolved later on worker threads. Run all worker-thread C++->Kotlin JNI work under facebook::jni::ThreadScope::WithClassLoader, which attaches the thread AND installs fbjni's cached app class loader for the duration, so every FindClass inside (incl. ArrayBuffer) resolves app classes. The new ECR17_RUN_ON_JVM_THREAD(body) macro wraps ensureConnected/runTransaction/ runAckOnly (#ifdef __ANDROID__ -> WithClassLoader; no-op inline on iOS), capturing the return value in an outer local and any exception via std::exception_ptr so the money-safety try/catch semantics are unchanged. Only reproducible by running the app; no build/CI catches it. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/LESSON.md | 20 +++ package/cpp/Ecr17Client/HybridEcr17Client.cpp | 156 +++++++++++------- 2 files changed, 116 insertions(+), 60 deletions(-) diff --git a/docs/LESSON.md b/docs/LESSON.md index 3bf1359..1681c71 100644 --- a/docs/LESSON.md +++ b/docs/LESSON.md @@ -53,6 +53,26 @@ `jclass` globally, so later method calls from worker threads work (they only need a `JNIEnv`, supplied by the `ThreadScope` guards). Only reproducible by RUNNING the app. (Alternative: `ThreadScope::WithClassLoader`.) +- **`ClassNotFoundException` for `com.margelo.nitro.core.ArrayBuffer` when a command + runs.** This is the SAME root cause as above but for a NitroModules *core* class, + not our transport. The generated transport bridge resolves `ArrayBuffer` LAZILY on + the worker thread inside `send()` (`JHybridEcr17TransportSpec::send` → + `JArrayBuffer::wrap` → `FindClass("com/margelo/nitro/core/ArrayBuffer")`). A plain + `facebook::jni::ThreadScope` only ATTACHES a JNIEnv — it does NOT install the app + class loader, so that `FindClass` still hits the SYSTEM loader + (`DexPathList[... /system/lib64 ...]`) → ClassNotFoundException. The PR#8 fix + (creating the transport on the JS thread) only cached OUR transport's jclass; it + doesn't help core classes looked up later on a worker thread. Fix: run ALL + worker-thread C++→Kotlin JNI work under **`facebook::jni::ThreadScope::WithClassLoader(std::function)`**, + which attaches the thread AND installs fbjni's cached app class loader for the + duration, so every `FindClass` inside (incl. NitroModules' ArrayBuffer) resolves + app classes. We wrap the bodies of `ensureConnected`/`runTransaction`/`runAckOnly` + with the `ECR17_RUN_ON_JVM_THREAD(body)` macro (`#ifdef __ANDROID__` → + `WithClassLoader`; no-op-inline on iOS). `WithClassLoader` takes a void lambda, so + the macro captures the return value in an outer local and any thrown exception via + `std::exception_ptr` (rethrown after the scope) to preserve the money-safety + try/catch semantics untouched. Replaces the old plain-`ThreadScope` + `ECR17_JNI_THREAD_GUARD`. Only reproducible by RUNNING the app — no build/CI catches it. - **Emit `DISCONNECTED` on a failed connect**, else listeners stay stuck on `CONNECTING`. `connect()` delegates to `ensureConnected()` which emits CONNECTING→(CONNECTED | DISCONNECTED on throw). diff --git a/package/cpp/Ecr17Client/HybridEcr17Client.cpp b/package/cpp/Ecr17Client/HybridEcr17Client.cpp index 74ab716..82ceb0e 100644 --- a/package/cpp/Ecr17Client/HybridEcr17Client.cpp +++ b/package/cpp/Ecr17Client/HybridEcr17Client.cpp @@ -14,15 +14,40 @@ #include "Session/RetryPolicy.hpp" // Commands run on Nitro's C++ thread pool. On Android, those worker threads are -// NOT attached to the JVM, so calling the Kotlin transport through the generated -// JNI bridge fails with "Unable to retrieve jni environment. Is the thread -// attached?". ECR17_JNI_THREAD_GUARD attaches the current thread for its scope -// (RAII) on Android; it's a no-op on iOS (no JVM). +// NOT attached to the JVM, and — even once attached — JNI `FindClass` on an +// attached worker thread resolves against the *system* class loader, which can't +// see app/NitroModules classes (e.g. com.margelo.nitro.core.ArrayBuffer, looked +// up lazily by the generated transport bridge in `send()` via JArrayBuffer::wrap). +// That yields "Unable to retrieve jni environment" (no JNIEnv) or +// "ClassNotFoundException ... DexPathList[... /system/lib64 ...]" (wrong loader). +// +// ECR17_RUN_ON_JVM_THREAD(body) runs `body` on Android under fbjni's +// ThreadScope::WithClassLoader, which attaches the thread AND installs fbjni's +// cached app class loader for the duration, so every FindClass inside (including +// NitroModules' ArrayBuffer lookup) resolves app classes. On iOS (no JVM) it just +// runs the body inline. WithClassLoader takes a `std::function`; this +// macro captures any thrown C++ exception via std::exception_ptr and rethrows it +// after the scope so callers' try/catch logic is preserved, and lets the body +// assign to outer locals for return values. #ifdef __ANDROID__ #include -#define ECR17_JNI_THREAD_GUARD ::facebook::jni::ThreadScope __ecr17JniScope +#define ECR17_RUN_ON_JVM_THREAD(body) \ + do { \ + std::exception_ptr __ecr17Err; \ + ::facebook::jni::ThreadScope::WithClassLoader([&]() { \ + try { \ + body \ + } catch (...) { \ + __ecr17Err = std::current_exception(); \ + } \ + }); \ + if (__ecr17Err) std::rethrow_exception(__ecr17Err); \ + } while (0) #else -#define ECR17_JNI_THREAD_GUARD ((void)0) +#define ECR17_RUN_ON_JVM_THREAD(body) \ + do { \ + body \ + } while (0) #endif namespace margelo::nitro::ecr17 { @@ -269,24 +294,26 @@ void HybridEcr17Client::ensureInit() { } void HybridEcr17Client::ensureConnected() { - ECR17_JNI_THREAD_GUARD; // attach this worker thread for the transport JNI calls - ensureInit(); - if (transport_->isConnected()) { - return; - } - // Auto-connect: block this worker thread until the native transport connects - // (or throw on failure). keepAlive leaves the socket open for reuse. - if (onConnectionStateChange_) onConnectionStateChange_(ConnectionState::CONNECTING); - const double port = config_.port.value_or(1024); - const double timeout = config_.connectionTimeoutMs.value_or(5000); - try { - transport_->connect(config_.host, port, timeout)->await().get(); - } catch (...) { - // Don't leave listeners stuck on CONNECTING when the connection fails. - if (onConnectionStateChange_) onConnectionStateChange_(ConnectionState::DISCONNECTED); - throw; - } - if (onConnectionStateChange_) onConnectionStateChange_(ConnectionState::CONNECTED); + // Run the transport JNI work under the app class loader (Android); inline on iOS. + ECR17_RUN_ON_JVM_THREAD({ + ensureInit(); + if (transport_->isConnected()) { + return; // exits the WithClassLoader lambda only — no further JNI needed + } + // Auto-connect: block this worker thread until the native transport connects + // (or throw on failure). keepAlive leaves the socket open for reuse. + if (onConnectionStateChange_) onConnectionStateChange_(ConnectionState::CONNECTING); + const double port = config_.port.value_or(1024); + const double timeout = config_.connectionTimeoutMs.value_or(5000); + try { + transport_->connect(config_.host, port, timeout)->await().get(); + } catch (...) { + // Don't leave listeners stuck on CONNECTING when the connection fails. + if (onConnectionStateChange_) onConnectionStateChange_(ConnectionState::DISCONNECTED); + throw; + } + if (onConnectionStateChange_) onConnectionStateChange_(ConnectionState::CONNECTED); + }); } std::string HybridEcr17Client::cashRegisterIdOr(const std::optional& override) const { @@ -296,7 +323,6 @@ std::string HybridEcr17Client::cashRegisterIdOr(const std::optional DecodedPacket HybridEcr17Client::runTransaction( const std::string& mainPayload, const std::optional& tokenization, bool safeToRetry) { - ECR17_JNI_THREAD_GUARD; // attach this worker thread for the transport JNI calls std::lock_guard txLock(txMutex_); // serialize exchanges on the shared session auto doExchange = [&]() -> DecodedPacket { if (tokenization.has_value()) { @@ -310,49 +336,59 @@ DecodedPacket HybridEcr17Client::runTransaction( return session_->exchange(mainPayload); }; - try { - return doExchange(); - } catch (const std::exception&) { - const auto originalError = std::current_exception(); - const bool autoReconnect = config_.autoReconnect.value_or(false); - const bool dropped = !transport_ || !transport_->isConnected(); - if (autoReconnect && dropped) { - try { - ensureConnected(); // restore the socket for subsequent commands - } catch (...) { - // Reconnect failed: surface the original exchange error, not the - // reconnect failure (the former is what the caller needs to see). - std::rethrow_exception(originalError); + // All exchange JNI (session_ -> adapter_ -> Kotlin transport, incl. the + // ArrayBuffer lookup in send()) must run under the app class loader on Android. + DecodedPacket result; + ECR17_RUN_ON_JVM_THREAD({ + try { + result = doExchange(); + return; + } catch (const std::exception&) { + const auto originalError = std::current_exception(); + const bool autoReconnect = config_.autoReconnect.value_or(false); + const bool dropped = !transport_ || !transport_->isConnected(); + if (autoReconnect && dropped) { + try { + ensureConnected(); // restore the socket for subsequent commands + } catch (...) { + // Reconnect failed: surface the original exchange error, not the + // reconnect failure (the former is what the caller needs to see). + std::rethrow_exception(originalError); + } } + if (shouldRetryAfterReconnect(autoReconnect, dropped, safeToRetry)) { + result = doExchange(); // only read-only/idempotent ops may be replayed + return; + } + throw; // financial op: surface the error (recover via sendLastResult / 'G') } - if (shouldRetryAfterReconnect(autoReconnect, dropped, safeToRetry)) { - return doExchange(); // only read-only/idempotent ops may be replayed - } - throw; // financial op: surface the error (recover via sendLastResult / 'G') - } + }); + return result; } void HybridEcr17Client::runAckOnly(const std::string& payload, bool safeToRetry) { - ECR17_JNI_THREAD_GUARD; // attach this worker thread for the transport JNI calls std::lock_guard txLock(txMutex_); // serialize exchanges on the shared session - try { - session_->sendAckOnly(payload); - } catch (const std::exception&) { - const auto originalError = std::current_exception(); - const bool autoReconnect = config_.autoReconnect.value_or(false); - const bool dropped = !transport_ || !transport_->isConnected(); - if (autoReconnect && dropped) { - try { - ensureConnected(); - } catch (...) { - std::rethrow_exception(originalError); // surface the original error + // Send under the app class loader on Android (ArrayBuffer lookup in send()). + ECR17_RUN_ON_JVM_THREAD({ + try { + session_->sendAckOnly(payload); + } catch (const std::exception&) { + const auto originalError = std::current_exception(); + const bool autoReconnect = config_.autoReconnect.value_or(false); + const bool dropped = !transport_ || !transport_->isConnected(); + if (autoReconnect && dropped) { + try { + ensureConnected(); + } catch (...) { + std::rethrow_exception(originalError); // surface the original error + } } + if (!shouldRetryAfterReconnect(autoReconnect, dropped, safeToRetry)) { + throw; // not retryable: surface the original error + } + session_->sendAckOnly(payload); // read-only/idempotent op: safe to replay } - if (!shouldRetryAfterReconnect(autoReconnect, dropped, safeToRetry)) { - throw; // not retryable: surface the original error - } - session_->sendAckOnly(payload); // read-only/idempotent op: safe to replay - } + }); } std::shared_ptr> HybridEcr17Client::connect() { From b532a0cca7e4617cf99066dd57f5b7db52b45ee1 Mon Sep 17 00:00:00 2001 From: "lorenzo.padovani@padosoft.com" Date: Thu, 28 May 2026 23:50:54 +0200 Subject: [PATCH 2/2] =?UTF-8?q?fix(android):=20address=20review=20?= =?UTF-8?q?=E2=80=94=20template=20helper=20instead=20of=20inline=20macro?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex (P1) + Copilot review of PR #10: - Codex P1: the previous ECR17_RUN_ON_JVM_THREAD *macro* expanded the body inline on iOS (no __ANDROID__), so the bare `return;` used to early-exit the Android WithClassLoader lambda was compiled inside the value-returning runTransaction -> invalid in a non-void function. Since Ecr17.podspec globs cpp/**, Apple builds would fail. Replace the macro with a template helper `runOnJvmThread(Fn)` that ALWAYS takes a callable: on Android it runs it via ThreadScope::WithClassLoader (capturing the result/exception, since WithClassLoader takes std::function), on iOS it just calls it. `return` inside the lambda now exits only the lambda on BOTH platforms, and value-returning callers `return runOnJvmThread(...)` directly. Verified both paths compile clean with g++ -std=c++20 -Wall -Wextra (with/without -D__ANDROID__). - Copilot: renamed the reserved double-underscore identifier `__ecr17Err` to `err` (reserved identifiers are implementation-only in C++). Money-safety try/catch semantics are unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) --- package/cpp/Ecr17Client/HybridEcr17Client.cpp | 89 +++++++++++-------- 1 file changed, 53 insertions(+), 36 deletions(-) diff --git a/package/cpp/Ecr17Client/HybridEcr17Client.cpp b/package/cpp/Ecr17Client/HybridEcr17Client.cpp index 82ceb0e..570f310 100644 --- a/package/cpp/Ecr17Client/HybridEcr17Client.cpp +++ b/package/cpp/Ecr17Client/HybridEcr17Client.cpp @@ -8,6 +8,8 @@ #include #include #include +#include +#include #include "Ecr17Protocol/Ecr17Protocol.hpp" #include "Ecr17Response/Ecr17Response.hpp" @@ -20,38 +22,57 @@ // up lazily by the generated transport bridge in `send()` via JArrayBuffer::wrap). // That yields "Unable to retrieve jni environment" (no JNIEnv) or // "ClassNotFoundException ... DexPathList[... /system/lib64 ...]" (wrong loader). -// -// ECR17_RUN_ON_JVM_THREAD(body) runs `body` on Android under fbjni's -// ThreadScope::WithClassLoader, which attaches the thread AND installs fbjni's -// cached app class loader for the duration, so every FindClass inside (including -// NitroModules' ArrayBuffer lookup) resolves app classes. On iOS (no JVM) it just -// runs the body inline. WithClassLoader takes a `std::function`; this -// macro captures any thrown C++ exception via std::exception_ptr and rethrows it -// after the scope so callers' try/catch logic is preserved, and lets the body -// assign to outer locals for return values. #ifdef __ANDROID__ #include -#define ECR17_RUN_ON_JVM_THREAD(body) \ - do { \ - std::exception_ptr __ecr17Err; \ - ::facebook::jni::ThreadScope::WithClassLoader([&]() { \ - try { \ - body \ - } catch (...) { \ - __ecr17Err = std::current_exception(); \ - } \ - }); \ - if (__ecr17Err) std::rethrow_exception(__ecr17Err); \ - } while (0) -#else -#define ECR17_RUN_ON_JVM_THREAD(body) \ - do { \ - body \ - } while (0) #endif namespace margelo::nitro::ecr17 { +namespace { + +// Runs `fn` on Android under fbjni's ThreadScope::WithClassLoader, which attaches +// the current thread to the JVM AND installs fbjni's cached app class loader for +// the duration — so every JNI FindClass inside (including NitroModules' lazy +// ArrayBuffer lookup) resolves app classes, not the system loader. On iOS (no JVM) +// `fn` is just called directly. Returns whatever `fn` returns (incl. void) and +// propagates exceptions, so the caller's try/catch and return-value logic is +// unchanged. WithClassLoader takes a `std::function`, so on Android the +// result is captured in a local and any exception via std::exception_ptr, then +// rethrown after the scope. `fn` is a lambda, so a `return` inside it returns from +// the lambda (not the caller) on BOTH platforms — safe in value-returning callers. +template +auto runOnJvmThread(Fn&& fn) -> decltype(fn()) { +#ifdef __ANDROID__ + using Ret = decltype(fn()); + std::exception_ptr err; + if constexpr (std::is_void_v) { + ::facebook::jni::ThreadScope::WithClassLoader([&]() { + try { + fn(); + } catch (...) { + err = std::current_exception(); + } + }); + if (err) std::rethrow_exception(err); + } else { + std::optional result; + ::facebook::jni::ThreadScope::WithClassLoader([&]() { + try { + result.emplace(fn()); + } catch (...) { + err = std::current_exception(); + } + }); + if (err) std::rethrow_exception(err); + return std::move(*result); + } +#else + return fn(); +#endif +} + +} // namespace + using margelo::nitro::HybridObjectRegistry; using margelo::nitro::Promise; @@ -295,10 +316,10 @@ void HybridEcr17Client::ensureInit() { void HybridEcr17Client::ensureConnected() { // Run the transport JNI work under the app class loader (Android); inline on iOS. - ECR17_RUN_ON_JVM_THREAD({ + runOnJvmThread([&]() { ensureInit(); if (transport_->isConnected()) { - return; // exits the WithClassLoader lambda only — no further JNI needed + return; // returns from this lambda only — no further JNI needed } // Auto-connect: block this worker thread until the native transport connects // (or throw on failure). keepAlive leaves the socket open for reuse. @@ -338,11 +359,9 @@ DecodedPacket HybridEcr17Client::runTransaction( // All exchange JNI (session_ -> adapter_ -> Kotlin transport, incl. the // ArrayBuffer lookup in send()) must run under the app class loader on Android. - DecodedPacket result; - ECR17_RUN_ON_JVM_THREAD({ + return runOnJvmThread([&]() -> DecodedPacket { try { - result = doExchange(); - return; + return doExchange(); } catch (const std::exception&) { const auto originalError = std::current_exception(); const bool autoReconnect = config_.autoReconnect.value_or(false); @@ -357,19 +376,17 @@ DecodedPacket HybridEcr17Client::runTransaction( } } if (shouldRetryAfterReconnect(autoReconnect, dropped, safeToRetry)) { - result = doExchange(); // only read-only/idempotent ops may be replayed - return; + return doExchange(); // only read-only/idempotent ops may be replayed } throw; // financial op: surface the error (recover via sendLastResult / 'G') } }); - return result; } void HybridEcr17Client::runAckOnly(const std::string& payload, bool safeToRetry) { std::lock_guard txLock(txMutex_); // serialize exchanges on the shared session // Send under the app class loader on Android (ArrayBuffer lookup in send()). - ECR17_RUN_ON_JVM_THREAD({ + runOnJvmThread([&]() { try { session_->sendAckOnly(payload); } catch (const std::exception&) {