QGrpcClientBase: remove errorOccurred()

Besides some clientbase specific error handling (i.e. thread check,
channel check) this signal didn't do much. It also piped through the
errors received from unary calls or streams.

We don't want to provide that signal anymore because:
    1. The checking (thread, channel) should provide the failure
       directly through the return value.
    2. Clients should just use the finished() signal.

This strengthen the already ongoing schema to make users responsible for
their RPC, including their lifetime. Lets continue on that!

Pick-to: 6.8
Change-Id: I28a4fca0ee0173dbf52b9c2d64d44ed8e5b37e9d
Reviewed-by:  Alexey Edelev <alexey.edelev@qt.io>

src/grpc/qgrpcclientbase.cpp

@@ -88,15 +88,6 @@ public:
     bool checkChannel();
     void addStream(QGrpcOperation *stream);
 
-    void sendErrorOccurredForNonMainThread(const char *methodName)
-    {
-        Q_Q(QGrpcClientBase);
-        QGrpcStatus s{ QtGrpc::StatusCode::Internal, threadSafetyWarning(methodName) };
-        auto method = QMetaMethod::fromSignal(&QGrpcClientBase::errorOccurred);
-        Q_ASSERT(method.isValid());
-        method.invoke(q, Qt::AutoConnection, std::move(s));
-    }
-
     std::shared_ptr<QAbstractProtobufSerializer> serializer() const {
         if (const auto &c = channel)
             return c->serializer();
@@ -112,7 +103,7 @@ bool QGrpcClientBasePrivate::checkThread(const char *methodName)
 {
     Q_Q(QGrpcClientBase);
     if (q->thread() != QThread::currentThread()) {
-        sendErrorOccurredForNonMainThread(methodName);
+        qGrpcWarning() << threadSafetyWarning(methodName);
         return false;
     }
     return true;
@@ -123,8 +114,7 @@ bool QGrpcClientBasePrivate::checkChannel()
     Q_Q(QGrpcClientBase);
 
     if (!channel) {
-        emit q->errorOccurred(QGrpcStatus{ QtGrpc::StatusCode::Unknown,
+        qGrpcWarning("No channel(s) attached");
-                                           q->tr("No channel(s) attached.") });
         return false;
     }
     return true;
@@ -136,20 +126,15 @@ void QGrpcClientBasePrivate::addStream(QGrpcOperation *grpcStream)
 
     Q_Q(QGrpcClientBase);
     // Remove the operation pointer upon QObject destruction if it hasn't
-    // already been gracefully removed by receiving the signals from below.
+    // already been gracefully removed by receiving finished()
     QObject::connect(grpcStream, &QObject::destroyed, q, [this, grpcStream](QObject *obj) {
         Q_ASSERT(obj == grpcStream);
         activeStreams.remove(grpcStream);
     });
 
-    // Transmit the signal from QGrpcOperation. If it emits, the transmission
-    // gets disconnected.
     auto finishedConnection = std::make_shared<QMetaObject::Connection>();
     *finishedConnection = QObject::connect(grpcStream, &QGrpcOperation::finished, q,
-                                           [this, grpcStream, q,
+                                           [this, grpcStream, finishedConnection] {
-                                            finishedConnection](const auto &status) mutable {
-                                               if (status != QtGrpc::StatusCode::Ok)
-                                                   Q_EMIT q->errorOccurred(status);
                                                Q_ASSERT(activeStreams.contains(grpcStream));
                                                activeStreams.remove(grpcStream);
                                                QObject::disconnect(*finishedConnection);
@@ -175,14 +160,15 @@ QGrpcClientBase::~QGrpcClientBase() = default;
     You have to invoke the channel-related functions on the same thread as
     QGrpcClientBase.
 */
-void QGrpcClientBase::attachChannel(std::shared_ptr<QAbstractGrpcChannel> channel)
+bool QGrpcClientBase::attachChannel(std::shared_ptr<QAbstractGrpcChannel> channel)
 {
     Q_D(QGrpcClientBase);
     // channel is not a QObject so we compare against the threadId set on construction.
     if (channel->dPtr->threadId != QThread::currentThreadId()) {
-        d->sendErrorOccurredForNonMainThread("attachChannel");
+        qGrpcWarning() << threadSafetyWarning("attachChannel");
-        return;
+        return false;
     }
+
     for (auto &stream : d->activeStreams) {
         assert(stream != nullptr);
         stream->cancel();
@@ -190,6 +176,7 @@ void QGrpcClientBase::attachChannel(std::shared_ptr<QAbstractGrpcChannel> channe
 
     d->channel = std::move(channel);
     emit channelChanged();
+    return true;
 }
 
 /*!
@@ -219,14 +206,6 @@ std::shared_ptr<QGrpcCallReply> QGrpcClientBase::call(QLatin1StringView method,
         return {};
 
     auto reply = d->channel->call(method, d->service, *argData, options);
-
-    auto errorConnection = std::make_shared<QMetaObject::Connection>();
-    *errorConnection = connect(reply.get(), &QGrpcCallReply::finished, this,
-                               [this, reply, errorConnection](const QGrpcStatus &status) {
-                                   if (status != QtGrpc::StatusCode::Ok)
-                                       emit errorOccurred(status);
-                                   QObject::disconnect(*errorConnection);
-                               });
     return reply;
 }
 
@@ -298,8 +277,7 @@ std::optional<QByteArray> QGrpcClientBase::trySerialize(const QProtobufMessage &
     Q_D(const QGrpcClientBase);
     auto serializer = d->serializer();
     if (serializer == nullptr) {
-        emit errorOccurred(QGrpcStatus{ QtGrpc::StatusCode::Unknown,
+        qGrpcWarning("Serializing failed. Serializer is not ready");
-                                        tr("Serializing failed. Serializer is not ready.") });
         return std::nullopt;
     }
 

src/grpc/qgrpcclientbase.h

@@ -44,11 +44,10 @@ class Q_GRPC_EXPORT QGrpcClientBase : public QObject
 public:
     ~QGrpcClientBase() override;
 
-    void attachChannel(std::shared_ptr<QAbstractGrpcChannel> channel);
+    bool attachChannel(std::shared_ptr<QAbstractGrpcChannel> channel);
     [[nodiscard]] std::shared_ptr<QAbstractGrpcChannel> channel() const noexcept;
 
 Q_SIGNALS:
-    void errorOccurred(const QGrpcStatus &status);
     void channelChanged();
 
 protected:

tests/auto/grpc/client/serverstream/tst_grpc_client_serverstream.cpp

@@ -36,7 +36,6 @@ private Q_SLOTS:
     void hugeBlob();
     void multipleStreams();
     void multipleStreamsCancel();
-    void inThread();
     void cancelWhileErrorTimeout();
 };
 
@@ -259,44 +258,6 @@ void QtGrpcClientServerStreamTest::multipleStreamsCancel()
     QCOMPARE_EQ(args.first().value<QGrpcStatus>().code(), QtGrpc::StatusCode::Ok);
 }
 
-void QtGrpcClientServerStreamTest::inThread()
-{
-    SimpleStringMessage result;
-    SimpleStringMessage request;
-    request.setTestFieldString("Stream");
-
-    QSignalSpy clientErrorSpy(client().get(), &TestService::Client::errorOccurred);
-    QVERIFY(clientErrorSpy.isValid());
-
-    int i = 0;
-    const std::unique_ptr<QThread> thread(QThread::create([&] {
-        QEventLoop waiter;
-        auto stream = client()->testMethodServerStream(request);
-        QObject::connect(stream.get(), &QGrpcServerStream::messageReceived, &waiter,
-                         [&result, &i, &waiter, stream] {
-                             const auto ret = stream->read<SimpleStringMessage>();
-                             QVERIFY(ret.has_value());
-                             result.setTestFieldString(result.testFieldString()
-                                                       + ret->testFieldString());
-                             if (++i == 4)
-                                 waiter.quit();
-                         });
-
-        waiter.exec();
-    }));
-
-    thread->start();
-
-    QTRY_COMPARE_EQ_WITH_TIMEOUT(clientErrorSpy.count(), 1, FailTimeout);
-    QTRY_VERIFY(result.testFieldString().isEmpty());
-    QTRY_VERIFY(
-            qvariant_cast<QGrpcStatus>(clientErrorSpy.at(0).first())
-                    .message()
-                    .startsWith(
-                            "QGrpcClientBase::startStream<QGrpcServerStream> is called from a "
-                            "different thread."));
-}
-
 void QtGrpcClientServerStreamTest::cancelWhileErrorTimeout()
 {
     SimpleStringMessage result;

tests/auto/grpc/client/ssl/tst_grpc_client_ssl.cpp

@@ -32,11 +32,9 @@ void QtGrpcSslClientTest::incorrectSecureCredentialsTest()
     SimpleStringMessage req;
     req.setTestFieldString("Hello Qt!");
 
-    QSignalSpy errorSpy(client().get(), &TestService::Client::errorOccurred);
     auto reply = client()->testMethod(req);
-    Q_UNUSED(reply)
+    QSignalSpy finishedSpy(reply.get(), &QGrpcCallReply::finished);
-
+    QTRY_COMPARE_EQ_WITH_TIMEOUT(finishedSpy.count(), 1, MessageLatencyWithThreshold);
-    QTRY_COMPARE_EQ_WITH_TIMEOUT(errorSpy.count(), 1, MessageLatencyWithThreshold);
 }
 
 QTEST_MAIN(QtGrpcSslClientTest)

tests/auto/grpc/client/unarycall/tst_grpc_client_unarycall.cpp

@@ -44,7 +44,6 @@ private Q_SLOTS:
     void deferredCancel();
     void asyncClientStatusMessage();
     void asyncStatusMessage();
-    void nonMainThreadIsInvalid();
     void metadata();
 };
 
@@ -80,22 +79,14 @@ void QtGrpcClientUnaryCallTest::immediateCancel()
     std::shared_ptr<QGrpcCallReply> reply = client()->testMethod(request);
 
     QSignalSpy replyFinishedSpy(reply.get(), &QGrpcCallReply::finished);
-    QSignalSpy clientErrorSpy(client().get(), &TestService::Client::errorOccurred);
-
     QVERIFY(replyFinishedSpy.isValid());
-    QVERIFY(clientErrorSpy.isValid());
 
     reply->cancel();
 
-    QTRY_COMPARE_EQ_WITH_TIMEOUT(clientErrorSpy.count(), 1, FailTimeout);
     QTRY_COMPARE_EQ_WITH_TIMEOUT(replyFinishedSpy.count(), 1, FailTimeout);
 
-    QCOMPARE_EQ(qvariant_cast<QGrpcStatus>(clientErrorSpy.at(0).first()).code(),
+    QCOMPARE_EQ(qvariant_cast<QGrpcStatus>(replyFinishedSpy.at(0).first()).code(),
                 QtGrpc::StatusCode::Cancelled);
-
-    auto args = replyFinishedSpy.first();
-    QCOMPARE(args.count(), 1);
-    QVERIFY(args.first().value<QGrpcStatus>() == QtGrpc::StatusCode::Cancelled);
 }
 
 void QtGrpcClientUnaryCallTest::deferredCancel()
@@ -121,15 +112,14 @@ void QtGrpcClientUnaryCallTest::asyncClientStatusMessage()
     SimpleStringMessage request;
     request.setTestFieldString("Some status message");
 
-    QSignalSpy clientErrorSpy(client().get(), &TestService::Client::errorOccurred);
-    QVERIFY(clientErrorSpy.isValid());
-
     auto reply = client()->testMethodStatusMessage(request);
-    Q_UNUSED(reply)
 
-    QTRY_COMPARE_EQ_WITH_TIMEOUT(clientErrorSpy.count(), 1, FailTimeout);
+    QSignalSpy replyFinishedSpy(reply.get(), &QGrpcCallReply::finished);
+    QVERIFY(replyFinishedSpy.isValid());
 
-    QCOMPARE(qvariant_cast<QGrpcStatus>(clientErrorSpy.at(0).first()).message(),
+    QTRY_COMPARE_EQ_WITH_TIMEOUT(replyFinishedSpy.count(), 1, FailTimeout);
+
+    QCOMPARE(qvariant_cast<QGrpcStatus>(replyFinishedSpy.at(0).first()).message(),
              request.testFieldString());
 }
 
@@ -149,32 +139,8 @@ void QtGrpcClientUnaryCallTest::asyncStatusMessage()
     QCOMPARE(args.first().value<QGrpcStatus>().message(), request.testFieldString());
 }
 
-void QtGrpcClientUnaryCallTest::nonMainThreadIsInvalid()
-{
-    SimpleStringMessage request;
-    request.setTestFieldString("Hello Qt from thread!");
-
-    QSignalSpy clientErrorSpy(client().get(), &TestService::Client::errorOccurred);
-    QVERIFY(clientErrorSpy.isValid());
-
-    const std::unique_ptr<QThread> thread(QThread::create([&] {
-        std::shared_ptr<QGrpcCallReply> reply = client()->testMethod(request);
-        QVERIFY(!reply);
-    }));
-
-    thread->start();
-    QTRY_COMPARE_EQ_WITH_TIMEOUT(clientErrorSpy.count(), 1, FailTimeout);
-    QTRY_VERIFY(
-            qvariant_cast<QGrpcStatus>(clientErrorSpy.at(0).first())
-                    .message()
-                    .startsWith("QGrpcClientBase::call is called from a different thread."));
-}
-
 void QtGrpcClientUnaryCallTest::metadata()
 {
-    QSignalSpy clientErrorSpy(client().get(), &TestService::Client::errorOccurred);
-    QVERIFY(clientErrorSpy.isValid());
-
     QGrpcCallOptions opt;
     opt.setMetadata({
         { "client_header",        "1"           },
@@ -205,7 +171,6 @@ void QtGrpcClientUnaryCallTest::metadata()
         }
     }
 
-    QCOMPARE_EQ(clientErrorSpy.count(), 0);
     QCOMPARE_EQ(serverHeaderCount, 1);
     QCOMPARE_EQ(clientReturnHeader, "valid_value"_ba);
 }

tests/auto/grpc/qgrpchttp2channel/tst_qgrpchttp2channel.cpp

@@ -54,6 +54,7 @@ class QGrpcHttp2ChannelTest : public QObject
 private Q_SLOTS:
     void checkMethodsGeneration();
     void attachChannelThreadTest();
+    void rpcThreadTest();
     void serializationFormat();
     void serializationFormatWithHeaders();
 };
@@ -71,28 +72,37 @@ void QGrpcHttp2ChannelTest::checkMethodsGeneration()
 
 void QGrpcHttp2ChannelTest::attachChannelThreadTest()
 {
-    std::shared_ptr<QGrpcHttp2Channel> channel;
+    std::shared_ptr<QGrpcHttp2Channel> threadChannel;
 
-    std::shared_ptr<QThread> thread(QThread::create([&] {
+    std::unique_ptr<QThread> thread(QThread::create([&] {
-        channel = std::make_shared<QGrpcHttp2Channel>(QUrl("http://localhost:50051",
+        threadChannel = std::make_shared<QGrpcHttp2Channel>(QUrl());
-                                                           QUrl::StrictMode));
     }));
     thread->start();
     thread->wait();
 
     TestService::Client client;
+    QVERIFY(!client.attachChannel(threadChannel));
+    QVERIFY(client.attachChannel(std::make_shared<QGrpcHttp2Channel>(QUrl())));
+}
 
-    QSignalSpy clientErrorSpy(&client, &TestService::Client::errorOccurred);
+void QGrpcHttp2ChannelTest::rpcThreadTest()
-    QVERIFY(clientErrorSpy.isValid());
+{
+    TestService::Client client;
+    client.attachChannel(std::make_shared<QGrpcHttp2Channel>(QUrl()));
 
-    client.attachChannel(channel);
+    std::unique_ptr<QThread> thread(QThread::create([&] {
+        QVERIFY(!client.testMethod(qtgrpc::tests::SimpleStringMessage()));
+        QVERIFY(!client.testMethodServerStream(qtgrpc::tests::SimpleStringMessage()));
+        QVERIFY(!client.testMethodClientStream(qtgrpc::tests::SimpleStringMessage()));
+        QVERIFY(!client.testMethodBiStream(qtgrpc::tests::SimpleStringMessage()));
+    }));
+    thread->start();
+    thread->wait();
 
-    QTRY_COMPARE_EQ(clientErrorSpy.count(), 1);
+    QVERIFY(client.testMethod(qtgrpc::tests::SimpleStringMessage()));
-    QCOMPARE(qvariant_cast<QGrpcStatus>(clientErrorSpy.at(0).first()).code(), StatusCode::Internal);
+    QVERIFY(client.testMethodServerStream(qtgrpc::tests::SimpleStringMessage()));
-    QVERIFY(qvariant_cast<QGrpcStatus>(clientErrorSpy.at(0).first())
+    QVERIFY(client.testMethodClientStream(qtgrpc::tests::SimpleStringMessage()));
-                .message()
+    QVERIFY(client.testMethodBiStream(qtgrpc::tests::SimpleStringMessage()));
-                .startsWith("QGrpcClientBase::attachChannel is called from a different "
-                            "thread."));
 }
 
 void QGrpcHttp2ChannelTest::serializationFormat()