diff --git a/README.md b/README.md index 1ba207d..560dd53 100644 --- a/README.md +++ b/README.md @@ -295,11 +295,32 @@ overloads differing only in `std::function` vs `std::function)>` are ambiguous for a generic lambda (`[](auto v){…}`), which would break existing call sites. -**Qt-free (`--api-style lp`) wrappers** spell the deadline `int timeout_ms = 0` -(`<= 0` selects the protocol default) because `Timeout` lives in a Qt header, -and they do **not** yet get `fooAsyncResult` — logos-protocol's -`lp_invoke_async` does not report the call error to its callback, so an -`AsyncResult` there would report success on a failed call. +**Qt-free (`--api-style lp`) wrappers** get all three entry points, spelling the +deadline `int timeout_ms = 0` (`<= 0` selects the protocol default) because +`Timeout` lives in a Qt header: + +```cpp +void fooAsyncResult(params…, std::function)> cb, + int timeout_ms = 0); +``` + +One asymmetry, deliberate: `fooAsync` on this surface takes no deadline. It has +existing callers and adding a parameter to it buys nothing that (3) does not +already give. + +`fooAsyncResult` was withheld here for a long time, and the reason is worth +knowing if you find a comment that still claims it: `lp_invoke_async` used to +hard-code `ok = 1`, so an `AsyncResult` over it would have reported success for +a call to a module that was not even loaded — an error channel that lies is +worse than none. logos-protocol#40 fixed that, and +`logos::LpClient::invokeAsyncResult` surfaces the failure in C++, so the twin is +honest. + +Both `foo(…, &err)` and `fooAsyncResult` on this surface also fold a provider +**rejection** into the error, matching the Qt path: a provider that ran and +refused answers `{"code": "dispatch_failed", …}` as its *result*, which the +return decode would otherwise erase into a default value. `fooAsync` still +cannot report it — its callback has nowhere to put it. ### Universal modules: LogosModuleContext diff --git a/cpp-generator/docs/project.md b/cpp-generator/docs/project.md index 3268c38..555086d 100644 --- a/cpp-generator/docs/project.md +++ b/cpp-generator/docs/project.md @@ -364,3 +364,12 @@ Fixture files in `tests/experimental/fixtures/`: callback still receives the default-converted value. `…AsyncResult` exists precisely because giving async an error channel was an API addition rather than a code-generation fix — a caller that needs to SEE the rejection uses it. + + The **Qt-free (`lp`) emitter** folds the same rejection through a `nlohmann::json` twin + of the detector (`logosDispatchRejectionJson`, under its own guard macro so both can + share a translation unit), into the same two surfaces: the sync `logos::CallError*` + out-parameter and `…AsyncResult`. Two differences from the Qt twin, both deliberate: + its sync path has no `qWarning` fallback for a caller that passed no `err` (a Qt-free + wrapper pulling in `` to say so would cost every generated TU for a + diagnostic nobody reads), and lp `…Async` is left alone for the same reason the Qt one + is — its callback takes the value alone. diff --git a/cpp-generator/generator_lib.cpp b/cpp-generator/generator_lib.cpp index 15afbd6..ff9dbb0 100644 --- a/cpp-generator/generator_lib.cpp +++ b/cpp-generator/generator_lib.cpp @@ -818,6 +818,42 @@ QString makeHeader(const QString& moduleName, const QString& className, const QJ // generated `_api.cpp`, so a module with more than one dependency puts // several of these in ONE translation unit. Internal linkage handles the // separate-TU case; only the preprocessor handles this one. +// The Qt-free twin of emitDispatchRejectionDetector, for the lp surface, whose +// results arrive as nlohmann::json rather than QVariant. Same exact match on +// the same three string fields and the same code, for the same reason: an `any` +// or map return carrying user data must never false-match. +// +// Guarded identically — the umbrella (`logos_sdk.cpp`) textually #includes every +// generated `_api.cpp`, so a module with more than one dependency puts +// several of these in ONE translation unit. +// +// The name matches logos-qt-sdk's plain-consumer backend +// (lidl_gen_qt_consumer.cpp), which emits a byte-identical helper: the two +// surfaces decode the same wire object, and one spelling means a TU that +// somehow sees both still compiles. +static void emitDispatchRejectionDetectorJson(QTextStream& s) +{ + s << "#ifndef LOGOS_GENERATED_DISPATCH_REJECTION_JSON\n"; + s << "#define LOGOS_GENERATED_DISPATCH_REJECTION_JSON\n\n"; + s << "namespace {\n\n"; + s << "// True when `v` is the canonical provider REJECTION object rather than a\n"; + s << "// value; fills `out` with its {code, message, origin} on a match.\n"; + s << "bool logosDispatchRejectionJson(const nlohmann::json& v, logos::CallError& out)\n"; + s << "{\n"; + s << " if (!v.is_object() || v.size() != 3) return false;\n"; + s << " auto code = v.find(\"code\"), message = v.find(\"message\"), origin = v.find(\"origin\");\n"; + s << " if (code == v.end() || message == v.end() || origin == v.end()) return false;\n"; + s << " if (!code->is_string() || !message->is_string() || !origin->is_string()) return false;\n"; + s << " if (code->get() != \"dispatch_failed\") return false;\n"; + s << " out.code = code->get();\n"; + s << " out.message = message->get();\n"; + s << " out.origin = origin->get();\n"; + s << " return true;\n"; + s << "}\n\n"; + s << "} // namespace\n\n"; + s << "#endif // LOGOS_GENERATED_DISPATCH_REJECTION_JSON\n\n"; +} + static void emitDispatchRejectionDetector(QTextStream& s) { s << "#ifndef LOGOS_GENERATED_DISPATCH_REJECTION\n"; @@ -1304,6 +1340,7 @@ QString makeHeaderLp(const QString& moduleName, const QString& className, const s << "#include \"logos_json.h\"\n"; s << "#include \"logos_result.h\"\n"; s << "#include \"logos_call_error.h\"\n"; + s << "#include \"logos_async_result.h\"\n"; s << "#include \"logos_lp_client.h\"\n"; // Record maps are std::map on the Qt-free surface. if (!rs.isEmpty()) s << "#include \n"; @@ -1351,20 +1388,24 @@ QString makeHeaderLp(const QString& moduleName, const QString& className, const // deadlines `int timeout_ms` with the C ABI's rule (`<= 0` selects the // protocol default), and this matches it. // - // NO `AsyncResult` HERE — deliberately, for now. The Qt surface gets - // one because its transport reports the error - // (LogosAPIClient::AsyncResultErrorCallback). This surface's transport does - // NOT: logos-protocol's lp_invoke_async (cpp/logos_protocol.cpp) subscribes - // with the VALUE-ONLY invokeRemoteMethodAsync overload and unconditionally - // calls back `cb(1, json, ...)` — ok is hard-coded to 1 — even though - // lp_result_cb is documented as "ok == 0 → `json` is the canonical error - // object", and even though its own sync twin lp_invoke does return - // LP_ERR_UNAVAILABLE + makeErrorJson. So an AsyncResult emitted here would - // report ok() on a failed call to a module that is not loaded: an error - // channel that lies is worse than no error channel. (Measured, not assumed: - // a wrapper wired to it fires its callback with the default value and an - // EMPTY error code.) Once lp_invoke_async reports the error, emitting the - // AsyncResult twin here is the same few lines as above. + // `AsyncResult` IS emitted here, matching the Qt surface. + // + // It was withheld for a long time, and the reason is worth recording because + // it was a property of the transport, not of this emitter: lp_invoke_async + // used to subscribe with the VALUE-ONLY invokeRemoteMethodAsync overload and + // hard-code `cb(1, json, ...)`, so a call to a module that is not loaded + // reached the callback as a SUCCESS carrying a default value. An AsyncResult + // built on that would have reported ok() for a failed call — an error + // channel that lies is worse than no error channel. logos-protocol#40 fixed + // it (logos_protocol.cpp now calls `cb(0, makeErrorJson(...))`), and + // logos::LpClient::invokeAsyncResult surfaces that in C++, so the twin is + // honest and the reason to withhold it is gone. + // + // The timeout is spelled the way the sync wrapper spells it (`int + // timeout_ms`, `<= 0` = protocol default) and is NEW rather than a + // regression of `Async`, which has never taken one: this method has no + // existing callers to keep compatible, and a fresh surface should not be + // born unable to state a deadline the client below already accepts. for (const QJsonValue& v : methods) { const QJsonObject o = v.toObject(); if (!o.value("isInvokable").toBool()) continue; @@ -1396,6 +1437,16 @@ QString makeHeaderLp(const QString& moduleName, const QString& className, const s << " void " << name << "Async("; emitDeclParams(); s << asyncCb << " callback);\n"; + + // Result-carrying async entry point. A DISTINCT NAME, not an overload + // of `Async`, for the same reason the Qt surface uses one: a + // generic lambda is convertible to BOTH std::function and + // std::function)>, so two overloads would be + // ambiguous at the call sites most likely to want the error. + s << " void " << name << "AsyncResult("; + emitDeclParams(); + s << "std::function)> callback, " + << "int timeout_ms = 0);\n"; } s << "\nprivate:\n"; @@ -1416,6 +1467,15 @@ QString makeSourceLp(const QString& moduleName, const QString& className, const QTextStream s(&c); s << "#include \"" << headerBaseName << "\"\n"; s << "#include \n\n"; + // Only reachable from a method body, so a contract with no invokable method + // must not emit it: an unused function in an anonymous namespace is a + // -Wunused-function warning, and such a wrapper stays byte-identical to + // what it generated before. + bool anyInvokable = false; + for (const QJsonValue& mv : methods) { + if (mv.toObject().value("isInvokable").toBool()) { anyInvokable = true; break; } + } + if (anyInvokable) emitDispatchRejectionDetectorJson(s); emitRecordConversions(s, rs, ApiStyle::Lp, className); // How the wrapper reaches its persistent LpClient + subscription store. @@ -1495,12 +1555,30 @@ QString makeSourceLp(const QString& moduleName, const QString& className, const if (!params.isEmpty()) s << ", "; s << "logos::CallError* err, int timeout_ms) {\n"; emitArgsArray(); - if (ret == "void") { - s << " " << clientExpr << ".invoke(\"" << name << "\", _args, err, timeout_ms);\n"; - } else { - s << " nlohmann::json _r = " << clientExpr << ".invoke(\"" << name << "\", _args, err, timeout_ms);\n"; + // Into a LOCAL, not straight into the caller's `err`: `err` is optional + // here (it defaults to nullptr) and the fold below needs somewhere to + // write regardless. The result is captured even for a `void` return — + // a void method can be rejected too, and the rejection object is the + // only place that says so. + s << " logos::CallError _err;\n"; + s << " nlohmann::json _r = " << clientExpr << ".invoke(\"" << name << "\", _args, &_err, timeout_ms);\n"; + // A provider that RAN and refused answers the canonical + // {"code":"dispatch_failed", …} object as its RESULT, not as a + // transport error, so LpClient::invoke reports ok() and the decode + // below turns the rejection into a default value — erasing it. Fold it + // into the same error channel the caller already reads, exactly as the + // Qt sync path does. + // + // No `else` warning branch, unlike the Qt twin: that one falls back to + // qWarning when the caller passed no `err`, and this surface has no + // logger to fall back to (a Qt-free wrapper that pulled in + // to say so would cost every generated TU for a diagnostic nobody + // reads). A caller that wants to know passes `&err` — which is the same + // deal this surface already offers for transport errors. + s << " if (_err.ok()) logosDispatchRejectionJson(_r, _err);\n"; + s << " if (err) *err = _err;\n"; + if (ret != "void") s << " return " << fromWireFor(qtRet, ApiStyle::Lp, rs, "_r", className + "::") << ";\n"; - } s << "}\n\n"; // Async @@ -1521,7 +1599,30 @@ QString makeSourceLp(const QString& moduleName, const QString& className, const } s << " });\n"; s << "}\n\n"; - // (No AsyncResult on this surface yet — see makeHeaderLp.) + + // Result-carrying async. Same arg marshalling and the SAME value + // decode as `Async` above, so a failed call delivers exactly the + // value that one would have delivered — plus the error that explains it. + s << "void " << className << "::" << name << "AsyncResult("; + emitParams(); + if (!params.isEmpty()) s << ", "; + s << "std::function)> callback, " + << "int timeout_ms) {\n"; + s << " if (!callback) return;\n"; + emitArgsArray(); + s << " " << clientExpr << ".invokeAsyncResult(\"" << name << "\", _args,\n"; + s << " [callback](nlohmann::json _r, const logos::CallError& _err) {\n"; + s << " logos::AsyncResult<" << ret << "> _res;\n"; + s << " _res.error = _err;\n"; + // Same fold as the sync path above, and for the same reason. + s << " if (_res.error.ok()) logosDispatchRejectionJson(_r, _res.error);\n"; + if (ret != "void") + s << " _res.value = " << fromWireFor(qtRet, ApiStyle::Lp, rs, "_r", className + "::") << ";\n"; + else + s << " (void)_r;\n"; + s << " callback(_res);\n"; + s << " }, timeout_ms);\n"; + s << "}\n\n"; } return c; } diff --git a/cpp/logos_lp_client.h b/cpp/logos_lp_client.h index 645caf1..29f5a05 100644 --- a/cpp/logos_lp_client.h +++ b/cpp/logos_lp_client.h @@ -15,6 +15,7 @@ // `LpSubscription` (mirrors rust-sdk's EventSubscription: unsubscribes on // destruction so the callback never fires after the owner is gone). +#include #include #include #include @@ -108,7 +109,10 @@ class LpClient { public: LpClient(std::string target, std::string origin) : m_target(std::move(target)), m_origin(std::move(origin)) {} - ~LpClient() { if (m_client) lp_client_destroy(m_client); } + ~LpClient() { + if (lp_client* c = m_client.load(std::memory_order_acquire)) + lp_client_destroy(c); + } LpClient(const LpClient&) = delete; LpClient& operator=(const LpClient&) = delete; @@ -164,6 +168,53 @@ public: &LpClient::resultTrampoline, box); } + // Async call carrying the error — the async twin of invoke()'s `err` + // out-parameter, and what the generated `AsyncResult` wrappers are + // built on. `cb` fires exactly once; on failure the JSON is null and the + // CallError is populated from the C ABI's canonical {code, message, origin} + // object. + // + // Why this exists next to invokeAsync rather than replacing it: invokeAsync + // collapses the C ABI's failure form (`ok == 0` with `json` set to the error + // object) into a bare JSON null, which is also what a successful call + // returning nothing delivers. That is fine for a callback that only takes a + // value and has nowhere to put an error, and useless for one that does. + // + // A DISTINCT NAME, not an overload of invokeAsync: two std::function + // parameters differing only in arity are ambiguous for a generic lambda — + // the same hazard that made the generator spell `AsyncResult` as its + // own name instead of an overload of `Async`. + // + // Safe to call from any thread. logos-qt-sdk's LpBridge::invokeAsyncResult + // is this function with a private second lp_client bolted on because this + // one did not exist; it can now delegate here and drop that connection. + void invokeAsyncResult(const std::string& method, + const nlohmann::json& args, + std::function cb, + int timeout_ms = 0) { + if (!cb) return; + lp_client* c = ensure(); + if (!c) { + cb(nlohmann::json(), + callErrorObjectUnavailable(m_target, "could not create client for " + m_target)); + return; + } + auto* box = new ResultErrBox(std::move(cb)); + const std::string argsStr = args.dump(); + const int rc = lp_invoke_async(c, method.c_str(), argsStr.c_str(), timeout_ms, + &LpClient::resultErrorTrampoline, box); + if (rc != LP_OK) { + // A synchronous refusal does NOT call back (the C ABI's rule), so + // the completion is this function's to make — `cb` still has to fire + // exactly once, which is the whole contract a caller schedules on. + ResultErrBox fn = std::move(*box); + delete box; + fn(nlohmann::json(), + callErrorCallFailed(m_target, "lp_invoke_async refused the call (rc=" + + std::to_string(rc) + ")")); + } + } + // The target's method list, as the JSON the host reports. Empty on // failure. Invoke-without-introspect is what makes a by-name call an // escape hatch rather than an API: a caller that cannot ask what exists @@ -193,11 +244,49 @@ public: private: using Box = std::function; + using ResultErrBox = std::function; + // Create-once, and never while holding a lock. + // + // Two threads reach a dep's FIRST call concurrently more often than the + // lazy-init shape suggests: a concurrency:"multi" module runs its handlers + // on concurrent QThreads, and any module with a worker of its own (an HTTP + // handler, a chain-sync pump) races that worker against the dispatch + // thread. The plain `if (!m_client) m_client = lp_client_create(...)` this + // replaces was a data race on m_client, and leaked whichever client lost. + // + // A mutex around the whole body is the obvious fix and the WRONG one. For a + // Qt-affine transport lp_client_create marshals construction onto the Qt + // main thread and BLOCKS there (logos_protocol.cpp's runOnQtMainThread). A + // worker holding the lock across that waits for the main thread — while the + // main thread, reaching this same ensure() from an inbound call, waits for + // the lock and so never returns to the event loop that would run the + // construction. That trades a data race for a deadlock. + // + // So construct OUTSIDE any lock and publish with a CAS. Both racers may + // build a client; exactly one is ever published, and the loser destroys its + // own. That is safe and cheap: lp_client_destroy may be called from any + // thread and defers the teardown to the owner thread (logos_protocol.h), + // and construction has no effect at the target — the capability handshake + // is lazy, inside invokeRemoteMethod — so a discarded client mints no token + // and leaves no trace. + // + // A failed create is deliberately NOT latched: the next call retries, which + // is what the pre-CAS version did. lp_client* ensure() { - if (!m_client) - m_client = lp_client_create(m_target.c_str(), m_origin.c_str(), nullptr, nullptr); - return m_client; + if (lp_client* c = m_client.load(std::memory_order_acquire)) + return c; + lp_client* fresh = lp_client_create(m_target.c_str(), m_origin.c_str(), nullptr, nullptr); + // Creation failed — report whatever is published (usually null, but a + // racer may have succeeded meanwhile) rather than caching the failure. + if (!fresh) return m_client.load(std::memory_order_acquire); + lp_client* expected = nullptr; + if (m_client.compare_exchange_strong(expected, fresh, + std::memory_order_acq_rel, + std::memory_order_acquire)) + return fresh; + lp_client_destroy(fresh); // lost the publish race + return expected; } static void resultTrampoline(int ok, const char* json, void* ud) { @@ -211,6 +300,35 @@ private: delete fn; // result callback fires exactly once } + // The error-aware twin of resultTrampoline. `ok == 0` means `json` is the + // canonical error object rather than a value, so the value is dropped and + // the error decoded; a malformed/absent one still yields a NON-ok + // CallError, because reporting ok() for a call the ABI said failed is the + // one outcome this trampoline exists to prevent. + static void resultErrorTrampoline(int ok, const char* json, void* ud) { + auto* fn = static_cast(ud); + nlohmann::json parsed; // null + if (json) { + auto p = nlohmann::json::parse(json, nullptr, /*allow_exceptions=*/false); + if (!p.is_discarded()) parsed = std::move(p); + } + CallError err; + if (!ok) { + err = callErrorCallFailed("", "lp_invoke_async failed"); + if (parsed.is_object()) { + if (parsed.contains("code") && parsed["code"].is_string()) + err.code = parsed["code"].get(); + if (parsed.contains("message") && parsed["message"].is_string()) + err.message = parsed["message"].get(); + if (parsed.contains("origin") && parsed["origin"].is_string()) + err.origin = parsed["origin"].get(); + } + parsed = nlohmann::json(); + } + (*fn)(std::move(parsed), err); + delete fn; // result callback fires exactly once + } + static void eventTrampoline(const char* /*eventName*/, const char* dataJson, void* ud) { auto* fn = static_cast(ud); nlohmann::json r = nlohmann::json::array(); @@ -240,7 +358,8 @@ private: std::string m_target; std::string m_origin; - lp_client* m_client = nullptr; + // Published exactly once by ensure(); read from any thread. + std::atomic m_client{nullptr}; }; } // namespace logos diff --git a/tests/generator/test_async_result.cpp b/tests/generator/test_async_result.cpp index 60557ea..3c8f9fa 100644 --- a/tests/generator/test_async_result.cpp +++ b/tests/generator/test_async_result.cpp @@ -83,9 +83,41 @@ TEST(SyncTimeout, LpBodyForwardsTimeoutMs) { const QString src = lpSource(); EXPECT_TRUE(src.contains("Mod::add(int64_t p0, int64_t p1, logos::CallError* err, int timeout_ms)")); - EXPECT_TRUE(src.contains("m_client.invoke(\"add\", _args, err, timeout_ms);")); - EXPECT_TRUE(src.contains("m_client.invoke(\"reset\", _args, err, timeout_ms);")); - EXPECT_FALSE(src.contains("_args, err);")); + // Into a LOCAL `_err`, then copied out: `err` is optional on this surface, + // and the dispatch-rejection fold needs somewhere to write either way. + EXPECT_TRUE(src.contains("m_client.invoke(\"add\", _args, &_err, timeout_ms);")); + EXPECT_TRUE(src.contains("m_client.invoke(\"reset\", _args, &_err, timeout_ms);")); + EXPECT_TRUE(src.contains("if (err) *err = _err;")); + // The deadline is still forwarded, never dropped for a fresh default. + EXPECT_FALSE(src.contains("_args, &_err);")); +} + +TEST(SyncTimeout, LpSyncFoldsAProviderRejectionIntoTheErrorChannel) +{ + // A provider that RAN and refused answers {"code":"dispatch_failed", …} as + // its RESULT, so LpClient::invoke reports ok() and the decode erases it. + // The Qt sync path has folded this for a while; this surface now does too. + const QString src = lpSource(); + const QString fold = "if (_err.ok()) logosDispatchRejectionJson(_r, _err);"; + ASSERT_TRUE(src.contains(fold)); + // Folded BEFORE the value is decoded and before `err` is written out, so a + // caller never reads an ok() error next to a default-decoded rejection. + const int f = src.indexOf(fold); + const int copy = src.indexOf("if (err) *err = _err;"); + const int ret = src.indexOf(" return (_r.is_number_integer()"); + ASSERT_NE(copy, -1); + ASSERT_NE(ret, -1); + EXPECT_LT(f, copy); + EXPECT_LT(copy, ret); +} + +TEST(SyncTimeout, LpVoidSyncStillCapturesTheResultSoItCanSeeARejection) +{ + // A void method can be rejected too, and the rejection object is the only + // place that says so — so the result is captured even where nothing is + // returned. `_r` is not unused: the fold reads it. + const QString src = lpSource(); + EXPECT_TRUE(src.contains("nlohmann::json _r = m_client.invoke(\"reset\", _args, &_err, timeout_ms);")); } // ─── 2. Async gains a result-carrying entry point ─────────────────────────── @@ -152,16 +184,109 @@ TEST(AsyncResult, ThePlainAsyncEntryPointIsUnchanged) EXPECT_TRUE(src.contains("[callback](QVariant v) {")); } -// ─── 4. The Qt-free surface deliberately has no AsyncResult yet ───────────── +// ─── 4. The Qt-free surface emits its own AsyncResult twin ────────────────── +// +// It was withheld for a long time, and for a reason that belonged to the +// transport rather than to this emitter: lp_invoke_async used to subscribe with +// the VALUE-ONLY overload and hard-code `cb(1, ...)`, so an AsyncResult built on +// it would have reported ok() for a call to a module that is not loaded. +// logos-protocol#40 fixed that (`cb(0, makeErrorJson(...))`) and +// logos::LpClient::invokeAsyncResult surfaces it in C++, so the twin is honest +// and is emitted. -TEST(AsyncResult, LpSurfaceDoesNotEmitAsyncResultWhileTheCAbiCannotReportOne) +TEST(AsyncResult, LpHeaderDeclaresTheDistinctlyNamedEntryPoint) { - // logos-protocol's lp_invoke_async hard-codes `cb(1, ...)`, so an - // AsyncResult here would report ok() on a failed call. See makeHeaderLp. - EXPECT_FALSE(lpHeader().contains("AsyncResult")); - EXPECT_FALSE(lpSource().contains("AsyncResult")); - // ...and the Lp async entry point keeps its exact shape. - EXPECT_TRUE(lpHeader().contains("void addAsync(int64_t p0, int64_t p1, std::function callback);")); + const QString h = lpHeader(); + EXPECT_TRUE(h.contains("void addAsyncResult(int64_t p0, int64_t p1, " + "std::function)> callback, " + "int timeout_ms = 0);")); + EXPECT_TRUE(h.contains("void nameAsyncResult(std::function)> callback, " + "int timeout_ms = 0);")); + // Same uniform shape the Qt surface uses for void: the error-only + // specialisation, never a bespoke callback. + EXPECT_TRUE(h.contains("void resetAsyncResult(std::function)> callback, " + "int timeout_ms = 0);")); + // `Timeout` lives in logos_mode.h, which includes . Naming it here + // would drag Qt into a translation unit whose whole purpose is not to have + // any, so this surface spells deadlines the way the C ABI does. + EXPECT_FALSE(h.contains("Timeout timeout")); +} + +TEST(AsyncResult, LpHeaderIncludesTheAsyncResultHeader) +{ + EXPECT_TRUE(lpHeader().contains("#include \"logos_async_result.h\"")); +} + +TEST(AsyncResult, LpBodyRoutesToTheErrorCarryingClientEntryPoint) +{ + const QString src = lpSource(); + // invokeAsyncResult, NOT invokeAsync: the latter collapses the C ABI's + // failure form into a bare JSON null, which is also what a successful call + // returning nothing delivers — indistinguishable, which is the whole defect. + EXPECT_TRUE(src.contains("m_client.invokeAsyncResult(\"add\", _args,")); + EXPECT_TRUE(src.contains("[callback](nlohmann::json _r, const logos::CallError& _err) {")); + EXPECT_TRUE(src.contains("logos::AsyncResult _res;")); + EXPECT_TRUE(src.contains("_res.error = _err;")); + EXPECT_TRUE(src.contains("callback(_res);")); + // The void form carries the error and nothing else. + EXPECT_TRUE(src.contains("logos::AsyncResult _res;")); +} + +TEST(AsyncResult, LpValueDecodeIsSharedWithThePlainAsyncEntryPoint) +{ + // Same decode expression in both, so a failed call delivers exactly the + // value `Async` would have delivered — plus the error. + const QString src = lpSource(); + const QString decode = "(_r.is_string() ? _r.get() : std::string())"; + EXPECT_TRUE(src.contains("callback(" + decode + ");")); + EXPECT_TRUE(src.contains("_res.value = " + decode + ";")); +} + +TEST(AsyncResult, LpRejectionIsFoldedBeforeTheValueIsDecoded) +{ + // Same rule as the sync path: fold before decoding, or this surface reports + // success for a refused call — on the one async surface that has somewhere + // to say otherwise. + const QString src = lpSource(); + const QString fold = "if (_res.error.ok()) logosDispatchRejectionJson(_r, _res.error);"; + ASSERT_TRUE(src.contains(fold)); + const int f = src.indexOf(fold); + const int decode = src.indexOf("_res.value = "); + const int deliver = src.indexOf("callback(_res);"); + ASSERT_NE(decode, -1); + ASSERT_NE(deliver, -1); + EXPECT_LT(f, decode); + EXPECT_LT(decode, deliver); +} + +TEST(AsyncResult, LpDispatchRejectionDetectorIsEmittedOnceAndGuarded) +{ + // The umbrella (logos_sdk.cpp) textually #includes EVERY generated + // _api.cpp, so a module with more than one dependency puts several of + // these in ONE translation unit. Internal linkage handles the separate-TU + // case; only the preprocessor handles this one. + const QString src = lpSource(); + EXPECT_EQ(src.count("bool logosDispatchRejectionJson"), 1); + EXPECT_TRUE(src.contains("#ifndef LOGOS_GENERATED_DISPATCH_REJECTION_JSON")); + EXPECT_TRUE(src.contains("#define LOGOS_GENERATED_DISPATCH_REJECTION_JSON")); +} + +TEST(AsyncResult, LpContractWithNoInvokableMethodEmitsNoDetector) +{ + // The detector is only reachable from a method body; emitting it anyway is + // an unused function in an anonymous namespace, i.e. -Wunused-function. + const QString src = makeSource("mod", "Mod", "mod.h", QJsonArray{}, ApiStyle::Lp); + EXPECT_FALSE(src.contains("logosDispatchRejectionJson")); +} + +TEST(AsyncResult, LpPlainAsyncEntryPointIsUnchanged) +{ + // No timeout was retro-fitted onto it — it has existing callers, and the + // twin is where the new capability goes. + const QString h = lpHeader(); + EXPECT_TRUE(h.contains("void addAsync(int64_t p0, int64_t p1, std::function callback);")); + EXPECT_TRUE(h.contains("void resetAsync(std::function callback);")); + EXPECT_TRUE(lpSource().contains("m_client.invokeAsync(\"add\", _args, [callback](nlohmann::json _r) {")); } // ─── 5. A REJECTION reaches the surface that can report it ────────────────── diff --git a/tests/generator/test_make_source.cpp b/tests/generator/test_make_source.cpp index f299485..325a11b 100644 --- a/tests/generator/test_make_source.cpp +++ b/tests/generator/test_make_source.cpp @@ -379,14 +379,23 @@ TEST(MakeSourceTest, QtNoInvokableMethodsEmitsNoDetector) EXPECT_FALSE(src.contains("logosDispatchRejection")); } -TEST(MakeSourceTest, LpSurfaceIsUntouched) +TEST(MakeSourceTest, LpSurfaceUsesItsOwnJsonDetectorNotTheQtOne) { - // The fix is Qt-consumer-only; the lp wrapper must generate exactly as - // before (byte-identical output is the negative control for the change). + // The Qt-consumer fix stays Qt-only: a QVariant-typed detector must never + // reach a translation unit whose whole purpose is not to see Qt. The lp + // surface folds the SAME wire object through its own nlohmann::json twin, + // on both the sync path and `AsyncResult` (see test_async_result.cpp). + // + // This test used to assert the lp wrapper contained no detector at all — + // the negative control for a fix that was Qt-only at the time. What it was + // really protecting is the Qt-freeness, which is what it asserts now. QJsonArray methods; methods.append(makeMethod("fn", "QVariantList", 1)); QString src = makeSource("mod", "Mod", "mod.h", methods, ApiStyle::Lp); - EXPECT_FALSE(src.contains("logosDispatchRejection")); + EXPECT_FALSE(src.contains("logosDispatchRejection(const QVariant&")); + EXPECT_TRUE(src.contains("bool logosDispatchRejectionJson(const nlohmann::json& v, logos::CallError& out)")); + // Its own guard macro, so the two detectors can coexist in one TU. + EXPECT_TRUE(src.contains("#ifndef LOGOS_GENERATED_DISPATCH_REJECTION_JSON")); } TEST(MakeSourceTest, QtRejectionDetectorIsPreprocessorGuarded) diff --git a/tests/sdk/CMakeLists.txt b/tests/sdk/CMakeLists.txt index efd9cd6..3a7b10e 100644 --- a/tests/sdk/CMakeLists.txt +++ b/tests/sdk/CMakeLists.txt @@ -8,13 +8,19 @@ add_executable(sdk_tests test_logos_module_context.cpp test_logos_host_services.cpp test_logos_host_core.cpp + test_lp_client.cpp ) # logos_host_services.h is a veneer over the lp_* C ABI, so this suite needs # logos-protocol's HEADERS. It deliberately does not need its LIBRARY: the # functions that call lp_* are `inline` and never ODR-used by these tests, so # nothing references a protocol symbol at link time. If a future test does call -# one, this will fail to LINK rather than silently pull the library in. +# one, this will fail to LINK rather than silently pull the library in — unless +# it supplies its own definition, which is exactly what test_lp_client.cpp does: +# it stubs lp_client_create/lp_client_destroy/lp_get_methods/lp_invoke_async so +# it can count clients, widen the create window, and drive each documented C ABI +# outcome — none of which the real library exposes. Stubs keep the no-library +# rule intact; adding the library here would not. # LOGOS_PROTOCOL_ROOT is the logos-protocol SOURCE tree here (nix/tests.nix # passes the flake input, not the built package), so the headers are under # cpp/ — a package would put them under include/. Accept either rather than diff --git a/tests/sdk/test_lp_client.cpp b/tests/sdk/test_lp_client.cpp new file mode 100644 index 0000000..e686a32 --- /dev/null +++ b/tests/sdk/test_lp_client.cpp @@ -0,0 +1,326 @@ +// logos::LpClient over STUBBED lp_* symbols — the two behaviours that are the +// wrapper's own rather than the transport's: when it creates its client, and how +// it decodes the C ABI's success/failure form. +// +// logos::LpClient creates its lp_client lazily, on whichever thread makes the +// first call through a generated `_api` wrapper. That thread is genuinely +// arbitrary and there can be more than one of it: a concurrency:"multi" module +// dispatches handlers on concurrent QThreads, and any module running a worker +// of its own (an HTTP handler, a chain-sync pump) races that worker against the +// dispatch thread on the very first call. +// +// Pre-fix, ensure() was `if (!m_client) m_client = lp_client_create(...)` — a +// data race on a plain pointer, with the losing racer's client leaked. This +// suite pins the replacement: construct outside any lock, publish with a CAS, +// and have the loser destroy its own client. +// +// The lp_* symbols below are LOCAL STUBS, not the real C ABI. sdk_tests +// deliberately does not link logos-protocol's library (see the CMakeLists), +// and the point here is the SDK-side logic, not the transport: the stubs let +// the test widen the create race, count exactly how many clients were built, +// published and destroyed, and drive lp_invoke_async through each of its +// documented outcomes — none of which the real library exposes. + +#include + +#include +#include +#include +#include +#include +#include +#include +#include + +#include "logos_lp_client.h" + +namespace { + +std::atomic g_created{0}; +std::atomic g_destroyed{0}; +// How many creates still have to fail before one is allowed to succeed. Models +// the deterministic-failure case (bad target/origin) the real +// lp_client_create() reports by returning NULL. +std::atomic g_failNext{0}; +// Widens the window between "created" and "published" so both racers really do +// build a client, rather than the test passing because one happened to win. +std::atomic g_slowCreate{false}; + +std::mutex g_seenMutex; +std::vector g_seen; // the client each getMethods() call observed +std::atomic g_stringsFreed{0}; + +// How the next lp_invoke_async should behave. Named for the C ABI outcome each +// one models, not for the test that uses it. +enum class AsyncStub { + Success, // ok != 0, `json` is the result value + FailWithError, // ok == 0, `json` is the canonical {code, message, origin} + FailMalformed, // ok == 0, `json` is not a usable error object + RefuseSync, // returns LP_ERR_INVALID_ARG and does NOT call back +}; +AsyncStub g_asyncStub = AsyncStub::Success; + +void resetStubs() { + g_created = 0; + g_destroyed = 0; + g_failNext = 0; + g_slowCreate = false; + g_asyncStub = AsyncStub::Success; + g_stringsFreed = 0; + std::lock_guard lock(g_seenMutex); + g_seen.clear(); +} + +} // namespace + +extern "C" { + +lp_client* lp_client_create(const char*, const char*, const char*, const char*) { + if (g_failNext.load() > 0 && g_failNext.fetch_sub(1) > 0) return nullptr; + g_created.fetch_add(1, std::memory_order_relaxed); + if (g_slowCreate.load()) std::this_thread::sleep_for(std::chrono::milliseconds(5)); + // lp_client is opaque; any distinct heap address stands in for one. + return reinterpret_cast(new std::uintptr_t(0xC0FFEEu)); +} + +void lp_client_destroy(lp_client* client) { + g_destroyed.fetch_add(1, std::memory_order_relaxed); + delete reinterpret_cast(client); +} + +// The cheapest public LpClient method that goes through ensure(). +// +// It returns a HEAP string the caller must hand back to lp_string_free, which +// is the real ABI contract — and stubbing it that way is load-bearing rather +// than cosmetic. This first returned NULL, which left getMethods()'s +// lp_string_free call unreachable: clang may inline this same-TU definition, +// prove the pointer null and delete the call, so a missing lp_string_free stub +// linked fine on macOS/clang and failed on GCC with an undefined reference. +// Returning a real allocation keeps that path live on every compiler. +char* lp_get_methods(lp_client* client) { + { + std::lock_guard lock(g_seenMutex); + g_seen.push_back(client); + } + char* out = static_cast(std::malloc(3)); + std::memcpy(out, "[]", 3); + return out; +} + +void lp_string_free(char* s) { + g_stringsFreed.fetch_add(1, std::memory_order_relaxed); + std::free(s); +} + +int lp_invoke_async(lp_client*, const char*, const char*, int, lp_result_cb cb, void* ud) { + switch (g_asyncStub) { + case AsyncStub::Success: + cb(1, "\"hi\"", ud); + return LP_OK; + case AsyncStub::FailWithError: + cb(0, "{\"code\":\"object_unavailable\",\"message\":\"not loaded\",\"origin\":\"target\"}", ud); + return LP_OK; + case AsyncStub::FailMalformed: + cb(0, "not json at all", ud); + return LP_OK; + case AsyncStub::RefuseSync: + // The ABI's rule: a synchronous argument/handle rejection does NOT + // call back. + return LP_ERR_INVALID_ARG; + } + return LP_OK; +} + +} // extern "C" + +class LpClientEnsureTest : public ::testing::Test { +protected: + void SetUp() override { resetStubs(); } + + static lp_client* soleSeenClient() { + std::lock_guard lock(g_seenMutex); + return g_seen.empty() ? nullptr : g_seen.front(); + } +}; + +TEST_F(LpClientEnsureTest, RepeatedCallsOnOneThreadBuildExactlyOneClient) { + { + logos::LpClient client("target", "origin"); + for (int i = 0; i < 5; ++i) client.getMethods(); + EXPECT_EQ(g_created.load(), 1); + EXPECT_EQ(g_destroyed.load(), 0); + // Every string the ABI handed out went back through lp_string_free — + // the ownership rule getMethods() has to honour, and the reason this + // stub returns a real allocation rather than NULL. + EXPECT_EQ(g_stringsFreed.load(), 5); + } + EXPECT_EQ(g_destroyed.load(), 1) << "the published client outlives every call, not the last one"; +} + +// The regression itself. Every thread must come away with the SAME client, and +// every client that was built but not published must have been destroyed — +// leaking one was the pre-fix behaviour whenever two threads first-called at +// once. +TEST_F(LpClientEnsureTest, ConcurrentFirstCallsPublishOneClientAndLeakNone) { + constexpr int kThreads = 8; + g_slowCreate = true; + + { + logos::LpClient client("target", "origin"); + + std::atomic ready{0}; + std::atomic go{false}; + std::vector threads; + threads.reserve(kThreads); + for (int i = 0; i < kThreads; ++i) { + threads.emplace_back([&]() { + ready.fetch_add(1); + while (!go.load(std::memory_order_acquire)) { /* spin: no sleep, keep the start tight */ } + client.getMethods(); + }); + } + while (ready.load() < kThreads) { /* spin */ } + go.store(true, std::memory_order_release); + for (std::thread& t : threads) t.join(); + + std::lock_guard lock(g_seenMutex); + ASSERT_EQ(static_cast(g_seen.size()), kThreads); + lp_client* published = g_seen.front(); + ASSERT_NE(published, nullptr); + for (lp_client* seen : g_seen) + EXPECT_EQ(seen, published) << "threads disagreed about which client is the module's"; + + // Losers destroy their own before returning the published one, so by + // the time every thread has joined the books already balance. + EXPECT_EQ(g_destroyed.load(), g_created.load() - 1) + << "created=" << g_created.load() << " destroyed=" << g_destroyed.load() + << " — a client was built and neither published nor destroyed"; + } + EXPECT_EQ(g_destroyed.load(), g_created.load()) << "the published client survived its owner"; +} + +// A create that fails is not cached: the real lp_client_create returns NULL for +// a bad target/origin, and latching that would turn one bad early call into a +// permanently dead dependency. +TEST_F(LpClientEnsureTest, AFailedCreateIsRetriedRatherThanLatched) { + logos::LpClient client("target", "origin"); + + g_failNext = 2; + client.getMethods(); + client.getMethods(); + EXPECT_EQ(g_created.load(), 0); + { + std::lock_guard lock(g_seenMutex); + EXPECT_TRUE(g_seen.empty()) << "a NULL client must not reach lp_get_methods"; + } + + client.getMethods(); + EXPECT_EQ(g_created.load(), 1); + EXPECT_NE(soleSeenClient(), nullptr); +} + +// ─── invokeAsyncResult: the error-carrying async ──────────────────────────── +// +// The generated `AsyncResult` wrappers are built on this, so what it +// reports IS what the error channel reports. invokeAsync, next to it, has +// nowhere to put an error and collapses every failure into a bare JSON null — +// which is also a successful call that returned nothing. These pin the +// difference. + +class LpClientAsyncResultTest : public LpClientEnsureTest {}; + +TEST_F(LpClientAsyncResultTest, SuccessDeliversTheValueAndAnOkError) { + logos::LpClient client("target", "origin"); + g_asyncStub = AsyncStub::Success; + + int calls = 0; + nlohmann::json got; + logos::CallError err; + err.code = "sentinel"; // must be overwritten, not merely left alone + client.invokeAsyncResult("m", nlohmann::json::array(), [&](nlohmann::json r, const logos::CallError& e) { + ++calls; got = std::move(r); err = e; + }); + + EXPECT_EQ(calls, 1); + EXPECT_TRUE(err.ok()) << err.code; + EXPECT_EQ(got, nlohmann::json("hi")); +} + +TEST_F(LpClientAsyncResultTest, FailureDeliversTheCanonicalErrorAndNoValue) { + logos::LpClient client("target", "origin"); + g_asyncStub = AsyncStub::FailWithError; + + int calls = 0; + nlohmann::json got = nlohmann::json("stale"); + logos::CallError err; + client.invokeAsyncResult("m", nlohmann::json::array(), [&](nlohmann::json r, const logos::CallError& e) { + ++calls; got = std::move(r); err = e; + }); + + EXPECT_EQ(calls, 1); + EXPECT_FALSE(err.ok()); + EXPECT_EQ(err.code, "object_unavailable"); + EXPECT_EQ(err.message, "not loaded"); + EXPECT_EQ(err.origin, "target"); + // The error object is NOT handed back as if it were a value — that is the + // distinction the plain invokeAsync path cannot make. + EXPECT_TRUE(got.is_null()); +} + +TEST_F(LpClientAsyncResultTest, AMalformedErrorObjectStillReportsNotOk) { + // Reporting ok() for a call the ABI said failed is the one outcome this + // entry point exists to prevent, so an undecodable error body must not + // degrade into success. + logos::LpClient client("target", "origin"); + g_asyncStub = AsyncStub::FailMalformed; + + int calls = 0; + logos::CallError err; + client.invokeAsyncResult("m", nlohmann::json::array(), [&](nlohmann::json, const logos::CallError& e) { + ++calls; err = e; + }); + + EXPECT_EQ(calls, 1); + EXPECT_FALSE(err.ok()); + EXPECT_EQ(err.code, "call_failed"); +} + +TEST_F(LpClientAsyncResultTest, ASynchronousRefusalStillCompletesExactlyOnce) { + // lp_invoke_async does NOT call back on LP_ERR_INVALID_ARG, so the + // completion is the wrapper's to make. A caller schedules on "fires exactly + // once"; silently never firing strands it. + logos::LpClient client("target", "origin"); + g_asyncStub = AsyncStub::RefuseSync; + + int calls = 0; + logos::CallError err; + client.invokeAsyncResult("m", nlohmann::json::array(), [&](nlohmann::json, const logos::CallError& e) { + ++calls; err = e; + }); + + EXPECT_EQ(calls, 1); + EXPECT_FALSE(err.ok()); + EXPECT_EQ(err.code, "call_failed"); + EXPECT_EQ(err.origin, "target"); +} + +TEST_F(LpClientAsyncResultTest, AnUncreatableClientCompletesRatherThanHanging) { + logos::LpClient client("target", "origin"); + g_failNext = 1000; // every create fails + + int calls = 0; + logos::CallError err; + client.invokeAsyncResult("m", nlohmann::json::array(), [&](nlohmann::json, const logos::CallError& e) { + ++calls; err = e; + }); + + EXPECT_EQ(calls, 1); + EXPECT_EQ(err.code, "object_unavailable"); + EXPECT_EQ(err.origin, "target"); +} + +TEST_F(LpClientAsyncResultTest, ANullCallbackIsANoOpRatherThanACall) { + logos::LpClient client("target", "origin"); + client.invokeAsyncResult("m", nlohmann::json::array(), nullptr); + EXPECT_EQ(g_created.load(), 0) << "a callback-less call must not even build a client"; +}