diff --git a/docs/LESSON.md b/docs/LESSON.md index 3bf1359..5df087e 100644 --- a/docs/LESSON.md +++ b/docs/LESSON.md @@ -53,6 +53,30 @@ `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` + by passing a lambda to the `runOnJvmThread(fn)` template helper: on Android it runs + `fn` inside `WithClassLoader`; on iOS (no JVM) it just calls `fn()`. Since + `WithClassLoader` takes a `std::function`, on Android the helper captures + `fn`'s return value in a `std::optional` and any thrown exception via + `std::exception_ptr` (rethrown after the scope), so the caller's return-value and + money-safety try/catch semantics are unchanged. `fn` being a lambda means a + `return` inside it exits the lambda (not the caller) on both platforms — safe in + the value-returning `runTransaction`. Replaces the old plain-`ThreadScope` + `ECR17_JNI_THREAD_GUARD` (which only attached a JNIEnv, not the class loader). + 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 9511553..86a2cdf 100644 --- a/package/cpp/Ecr17Client/HybridEcr17Client.cpp +++ b/package/cpp/Ecr17Client/HybridEcr17Client.cpp @@ -8,25 +8,71 @@ #include #include #include +#include +#include #include "Ecr17Protocol/Ecr17Protocol.hpp" #include "Ecr17Response/Ecr17Response.hpp" #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). #ifdef __ANDROID__ #include -#define ECR17_JNI_THREAD_GUARD ::facebook::jni::ThreadScope __ecr17JniScope -#else -#define ECR17_JNI_THREAD_GUARD ((void)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; @@ -269,24 +315,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(10000); - 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. + runOnJvmThread([&]() { + ensureInit(); + if (transport_->isConnected()) { + 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. + if (onConnectionStateChange_) onConnectionStateChange_(ConnectionState::CONNECTING); + const double port = config_.port.value_or(10000); + 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 +344,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 +357,55 @@ 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. + return runOnJvmThread([&]() -> DecodedPacket { + 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); + } } + 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') } - 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') - } + }); } 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()). + runOnJvmThread([&]() { + 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() {