mirror of
https://github.com/logos-co/logos-cpp-sdk.git
synced 2026-08-31 01:31:10 +00:00
Address review: async marshaling, helper constraints, test ownership
- invokeRemoteMethodAsync now also marshals to the owner thread (non-blocking QueuedConnection) — the async path acquires a replica too, so calling it from a worker thread previously re-introduced the off-thread bug. - runOnOwnerThread: document the return-type constraints (void or default-constructible, non-reference) and static_assert against references. - test: declare the provider before its LogosAPI so the ModuleProxy (which holds a raw pointer to it) is torn down first — removes the leak and the inaccurate comment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
43532db796
commit
c6e909673e
@@ -144,6 +144,21 @@ void LogosAPIClient::invokeRemoteMethodAsync(const QString& objectName, const QS
|
||||
{
|
||||
if (!callback) return;
|
||||
|
||||
// The async path acquires a replica too, so it must also run on the owner
|
||||
// thread. Unlike the sync path we post non-blocking (QueuedConnection): the
|
||||
// worker caller returns immediately and the result callback fires on the
|
||||
// owner thread when the reply arrives.
|
||||
if (QThread::currentThread() != this->thread()) {
|
||||
QMetaObject::invokeMethod(this,
|
||||
[this, objectName, methodName, args,
|
||||
callback = std::move(callback), timeout]() mutable {
|
||||
invokeRemoteMethodAsync(objectName, methodName, args,
|
||||
std::move(callback), timeout);
|
||||
},
|
||||
Qt::QueuedConnection);
|
||||
return;
|
||||
}
|
||||
|
||||
QString token = getToken(objectName);
|
||||
|
||||
if (token.isEmpty() && objectName != "capability_module" && m_capability_consumer) {
|
||||
|
||||
@@ -19,13 +19,20 @@ namespace logos {
|
||||
// thread) without the module touching Qt — the SDK transparently marshals the
|
||||
// call onto the owner thread.
|
||||
//
|
||||
// Requirements: `obj`'s thread must be running an event loop (it is — the
|
||||
// module's main thread runs QCoreApplication::exec()). The same-thread guard
|
||||
// avoids the BlockingQueuedConnection self-deadlock.
|
||||
// Requirements:
|
||||
// - `obj`'s thread must be running an event loop (it is — the module's main
|
||||
// thread runs QCoreApplication::exec()). The same-thread guard avoids the
|
||||
// BlockingQueuedConnection self-deadlock.
|
||||
// - The return type must be void or default-constructible (the marshaled
|
||||
// branch holds the result in a local before assigning it), and must not be
|
||||
// a reference (there'd be nothing to bind the local to). Both are satisfied
|
||||
// by the SDK's uses here (void, QVariant, LogosObject*, LogosAPIClient*).
|
||||
template <typename Fn>
|
||||
auto runOnOwnerThread(QObject* obj, Fn&& fn) -> decltype(fn())
|
||||
{
|
||||
using Ret = decltype(fn());
|
||||
static_assert(!std::is_reference_v<Ret>,
|
||||
"runOnOwnerThread does not support reference return types");
|
||||
if (QThread::currentThread() == obj->thread()) {
|
||||
return fn();
|
||||
}
|
||||
|
||||
@@ -76,10 +76,15 @@ TEST_F(WorkerThreadIpcTest, InvokeFromWorkerThreadRunsOnOwnerThread)
|
||||
{
|
||||
QThread* const ownerThread = QThread::currentThread();
|
||||
|
||||
// The ModuleProxy created by registerObject (owned by providerApi) keeps a
|
||||
// raw pointer to the provider while published, so the provider must outlive
|
||||
// it. Declaring `provider` before `providerApi` ensures that: at end of
|
||||
// scope, providerApi (and its proxy) is destroyed first, then the provider.
|
||||
ThreadProbeProvider provider;
|
||||
|
||||
// Provider registered on the owner thread.
|
||||
LogosAPI providerApi("thread_probe");
|
||||
auto* provider = new ThreadProbeProvider();
|
||||
ASSERT_TRUE(providerApi.getProvider()->registerObject("thread_probe", provider));
|
||||
ASSERT_TRUE(providerApi.getProvider()->registerObject("thread_probe", &provider));
|
||||
// Authorize the token the consumer will present, so the call reaches the
|
||||
// provider instead of being rejected by the authz check.
|
||||
providerApi.getProvider()->saveToken("caller", "tok");
|
||||
@@ -117,10 +122,6 @@ TEST_F(WorkerThreadIpcTest, InvokeFromWorkerThreadRunsOnOwnerThread)
|
||||
|
||||
// The crux: the inter-module call must execute on the owner thread, not the
|
||||
// worker thread. Without the marshaling fix it runs on the worker thread.
|
||||
EXPECT_EQ(provider->calledThread.load(), ownerThread)
|
||||
EXPECT_EQ(provider.calledThread.load(), ownerThread)
|
||||
<< "inter-module call executed on the worker thread instead of the owner thread";
|
||||
|
||||
// provider is intentionally leaked: the ModuleProxy created by
|
||||
// registerObject (owned by providerApi) references it for the lifetime of
|
||||
// the LogosAPI, which outlives this scope.
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user