#136 "Errno"-like error handling in clients is replaced by error-signal emition

Unito
Viktor ha unito 4 commit da semlanik/feature_qgrpc_error_handling_108 a semlanik/master 5 anni fa

+ 1 - 1
CMakeLists.txt

@@ -43,7 +43,7 @@ if(UNIX)
       set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Werror -Wno-pessimizing-move -Wno-mismatched-tags -Wno-unused-private-field")
     elseif ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
       # using GCC
-      set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Werror")
+      set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Werror -Wno-error=deprecated-declarations")
     endif()
 elseif(WIN32)
     if (${CMAKE_CXX_COMPILER_ID} MATCHES "MSVC")

+ 3 - 3
src/grpc/abstractchannel.h

@@ -40,9 +40,9 @@ class QTGRPCSHARED_EXPORT AbstractChannel
 {
 public:
     /*!
-     * \brief The Channel StatusCodes
+     * \brief Channel's status codes
      */
-    enum StatusCodes {
+    enum StatusCode {
         Ok = 0,                 //!< No error
         Cancelled = 1,          //!< The operation was cancelled, typically by the caller
         Unknown = 2,            //!< Unknown error
@@ -70,7 +70,7 @@ public:
      * \param ret
      * \return
      */
-    virtual StatusCodes call(const QString &method, const QString &service, const QByteArray &args, QByteArray &ret) = 0;
+    virtual StatusCode call(const QString &method, const QString &service, const QByteArray &args, QByteArray &ret) = 0;
 
     /*!
      * \brief call

+ 24 - 45
src/grpc/abstractclient.cpp

@@ -30,13 +30,10 @@
 namespace qtprotobuf {
 class AbstractClientPrivate final {
 public:
-    AbstractClientPrivate(const QString &service) : service(service)
-    , lastError(AbstractChannel::StatusCodes::Ok) {}
+    AbstractClientPrivate(const QString &service) : service(service) {}
 
     std::shared_ptr<AbstractChannel> channel;
     const QString service;
-    AbstractChannel::StatusCodes lastError;
-    QString lastErrorString;
 };
 }
 
@@ -45,7 +42,6 @@ using namespace qtprotobuf;
 AbstractClient::AbstractClient(const QString &service, QObject *parent) : QObject(parent)
   , d(new AbstractClientPrivate(service))
 {
-
 }
 
 AbstractClient::~AbstractClient()
@@ -58,63 +54,46 @@ void AbstractClient::attachChannel(const std::shared_ptr<AbstractChannel> &chann
     d->channel = channel;
 }
 
-bool AbstractClient::call(const QString &method, const QByteArray &arg, QByteArray &ret)
+AbstractChannel::StatusCode AbstractClient::call(const QString &method, const QByteArray &arg, QByteArray &ret)
 {
-    if (!d->channel) {
-        d->lastError = AbstractChannel::Unknown;
-        return false;
+    AbstractChannel::StatusCode callStatus = AbstractChannel::Unknown;
+    if (d->channel) {
+        callStatus = d->channel->call(method, d->service, arg, ret);
+    } else {
+        emit error(callStatus, QLatin1String("No channel(s) attached."));
     }
 
-    d->lastError = d->channel->call(method, d->service, arg, ret);
-    return d->lastError == AbstractChannel::Ok;
+    return callStatus;
 }
 
 AsyncReply *AbstractClient::call(const QString &method, const QByteArray &arg)
 {
-    AsyncReply *reply = new AsyncReply(d->channel, this);
+    AsyncReply *reply = nullptr;
+    if (d->channel) {
+        reply = new AsyncReply(d->channel, this);
 
-    if (!d->channel) {
-        d->lastError = AbstractChannel::Unknown;
-        d->lastErrorString = "No channel attached";
-        QTimer::singleShot(0, this, [reply]() {
-            reply->error(AbstractChannel::Unknown);
+        connect(reply, &AsyncReply::error, this, [this, reply](AbstractChannel::StatusCode statusCode) {
+            emit error(statusCode, QLatin1String("Connection has been aborted."));
             reply->deleteLater();
         });
-        return reply;
-    }
 
-    connect(reply, &AsyncReply::error, this, [this, reply](AbstractChannel::StatusCodes statusCode){
-        d->lastError = statusCode;
-        reply->deleteLater();
-    });
+        connect(reply, &AsyncReply::finished, this, [reply](){
+            reply->deleteLater();
+        });
 
-    connect(reply, &AsyncReply::finished, this, [this, reply](){
-        reply->deleteLater();
-    });
+        d->channel->call(method, d->service, arg, reply);
+    } else {
+        emit error(AbstractChannel::Unknown, QLatin1String("No channel(s) attached."));
+    }
 
-    d->lastError = AbstractChannel::Ok;//Assume that all is OK until something happened
-    d->channel->call(method, d->service, arg, reply);
     return reply;
 }
 
 void AbstractClient::subscribe_p(const QString &method, const QByteArray &arg, const std::function<void(const QByteArray&)> &handler)
 {
-    d->lastError = AbstractChannel::Ok;
-    if (!d->channel) {
-        d->lastError = AbstractChannel::Unknown;
-        d->lastErrorString = "No channel attached";
-        return;
+    if (d->channel) {
+        d->channel->subscribe(method, d->service, arg, this, handler);
+    } else {
+        emit error(AbstractChannel::Unknown, QLatin1String("No channel(s) attached."));
     }
-
-    d->channel->subscribe(method, d->service, arg, this, handler);
-}
-
-AbstractChannel::StatusCodes AbstractClient::lastError() const
-{
-    return d->lastError;
-}
-
-QString AbstractClient::lastErrorString() const
-{
-    return d->lastErrorString;
 }

+ 14 - 6
src/grpc/abstractclient.h

@@ -42,20 +42,27 @@
 namespace qtprotobuf {
 
 class AbstractChannel;
-class AbstractClientPrivate;
 
 class QTGRPCSHARED_EXPORT AbstractClient : public QObject
 {
+    Q_OBJECT
 public:
     void attachChannel(const std::shared_ptr<AbstractChannel> &channel);
 
-    AbstractChannel::StatusCodes lastError() const;
-    QString lastErrorString() const;
+signals:
+    void error(AbstractChannel::StatusCode code, const QString &errorText);
 
 protected:
     AbstractClient(const QString &service, QObject *parent = nullptr);
     virtual ~AbstractClient();
 
+    /**
+     * @brief A template of a sync RPC call
+     *
+     * @param method Name of the method to be called
+     * @param arg Arguments (proto-message) passed to the method called
+     * @param ret A pointer to memory with proto-message to write an RPC reply to
+     */
     template<typename A, typename R>
     bool call(const QString &method, const A &arg, const QPointer<R> &ret) {
         if (ret.isNull()) {
@@ -64,7 +71,7 @@ protected:
         }
 
         QByteArray retData;
-        if (call(method, arg.serialize(), retData)) {
+        if (AbstractChannel::StatusCode::Ok == call(method, arg.serialize(), retData)) {
             try {
                 ret->deserialize(retData);
             } catch (std::invalid_argument &) {
@@ -114,7 +121,7 @@ protected:
     }
 
 private:
-    bool call(const QString &method, const QByteArray &arg, QByteArray &ret);
+    AbstractChannel::StatusCode call(const QString &method, const QByteArray &arg, QByteArray &ret);
     AsyncReply *call(const QString &method, const QByteArray &arg);
     void subscribe_p(const QString &method, const QByteArray &arg, const std::function<void(const QByteArray &)> &handler);
 
@@ -122,7 +129,8 @@ private:
     AbstractClient(AbstractClient &&) = delete;
     AbstractClient &operator =(AbstractClient &&) = delete;
 
-    AbstractClientPrivate *d;
+    // PIMPL
+    class AbstractClientPrivate *d;
 };
 
 }

+ 1 - 1
src/grpc/asyncreply.h

@@ -56,7 +56,7 @@ public:
 
 signals:
     void finished();
-    void error(AbstractChannel::StatusCodes);
+    void error(AbstractChannel::StatusCode);
 
 protected:
     AsyncReply(const std::shared_ptr<AbstractChannel> &channel, QObject *parent = nullptr) : QObject(parent)

+ 20 - 17
src/grpc/http2channel.cpp

@@ -47,7 +47,13 @@
 using namespace qtprotobuf;
 
 namespace  {
-const static std::unordered_map<QNetworkReply::NetworkError, AbstractChannel::StatusCodes> StatusCodeMap = { { QNetworkReply::ConnectionRefusedError, AbstractChannel::Unavailable },
+
+/**
+ * This QNetworkReply::NetworkError -> AbstractChannel::StatusCode mapping should be kept in sync with original
+ * <a href="https://github.com/grpc/grpc/blob/master/doc/statuscodes.md">gRPC status codes</a>
+ */
+const static std::unordered_map<QNetworkReply::NetworkError, AbstractChannel::StatusCode> StatusCodeMap = {
+                                                                { QNetworkReply::ConnectionRefusedError, AbstractChannel::Unavailable },
                                                                 { QNetworkReply::RemoteHostClosedError, AbstractChannel::Unavailable },
                                                                 { QNetworkReply::HostNotFoundError, AbstractChannel::Unavailable },
                                                                 { QNetworkReply::TimeoutError, AbstractChannel::DeadlineExceeded },
@@ -148,18 +154,16 @@ struct Http2ChannelPrivate {
         }
     }
 
-    static QByteArray processReply(QNetworkReply *networkReply, AbstractChannel::StatusCodes &statusCode) {
+    static QByteArray processReply(QNetworkReply *networkReply, AbstractChannel::StatusCode &statusCode) {
         //Check if no network error occured
         if (networkReply->error() != QNetworkReply::NoError) {
-            qProtoWarning() << networkReply->error() << ":" << networkReply->errorString();
             statusCode = StatusCodeMap.at(networkReply->error());
             return {};
         }
 
         //Check if server answer with error
-        statusCode = static_cast<AbstractChannel::StatusCodes>(networkReply->rawHeader(GrpcStatusHeader).toInt());
-        if (statusCode != AbstractChannel::StatusCodes::Ok) {
-            qProtoWarning() << "Protobuf server error occured" << networkReply->errorString();
+        statusCode = static_cast<AbstractChannel::StatusCode>(networkReply->rawHeader(GrpcStatusHeader).toInt());
+        if (statusCode != AbstractChannel::StatusCode::Ok) {
             return {};
         }
 
@@ -214,7 +218,7 @@ Http2Channel::~Http2Channel()
     delete d;
 }
 
-AbstractChannel::StatusCodes Http2Channel::call(const QString &method, const QString &service, const QByteArray &args, QByteArray &ret)
+AbstractChannel::StatusCode Http2Channel::call(const QString &method, const QString &service, const QByteArray &args, QByteArray &ret)
 {
     QEventLoop loop;
 
@@ -226,7 +230,7 @@ AbstractChannel::StatusCodes Http2Channel::call(const QString &method, const QSt
         loop.exec();
     }
 
-    StatusCodes grpcStatus = StatusCodes::Unknown;
+    StatusCode grpcStatus = StatusCode::Unknown;
     ret = d->processReply(networkReply, grpcStatus);
 
     qProtoDebug() << __func__ << "RECV: " << ret.toHex() << "grpcStatus" << grpcStatus;
@@ -238,20 +242,20 @@ void Http2Channel::call(const QString &method, const QString &service, const QBy
     QNetworkReply *networkReply = d->post(method, service, args);
 
     auto connection = QObject::connect(networkReply, &QNetworkReply::finished, reply, [reply, networkReply]() {
-        StatusCodes grpcStatus = StatusCodes::Unknown;
+        StatusCode grpcStatus = StatusCode::Unknown;
         QByteArray data = Http2ChannelPrivate::processReply(networkReply, grpcStatus);
+
         qProtoDebug() << "RECV: " << data;
-        if (grpcStatus != StatusCodes::Ok) {
+        if (StatusCode::Ok == grpcStatus ) {
+            reply->setData(data);
+            reply->finished();
+        } else {
             reply->setData({});
             reply->error(grpcStatus);
-            reply->finished();
-            return;
         }
-        reply->setData(data);
-        reply->finished();
     });
 
-    QObject::connect(reply, &AsyncReply::error, networkReply, [reply, networkReply, connection]() {
+    QObject::connect(reply, &AsyncReply::error, networkReply, [networkReply, connection](AbstractChannel::StatusCode code) {
         QObject::disconnect(connection);
         Http2ChannelPrivate::abortNetworkReply(networkReply);
     });
@@ -312,6 +316,5 @@ void Http2Channel::abort(AsyncReply *reply)
 {
     assert(reply != nullptr);
     reply->setData({});
-    reply->error(StatusCodes::Aborted);
-    reply->finished();
+    reply->error(StatusCode::Aborted);
 }

+ 5 - 4
src/grpc/http2channel.h

@@ -31,18 +31,17 @@
 
 namespace qtprotobuf {
 
-struct Http2ChannelPrivate;
 class AbstractCredentials;
 
 class QTGRPCSHARED_EXPORT Http2Channel final : public AbstractChannel
 {
 public:
-    // this contructor is obsolete and is going to be removed soon
+    [[deprecated ("This contructor is obsolete and is going to be removed soon. Use Http2Channel(const QUrl&, const AbstractCredentials&)")]]
     Http2Channel(const QString &addr, quint16 port, const AbstractCredentials &credentials);
     Http2Channel(const QUrl &url, const AbstractCredentials &credentials);
     ~Http2Channel();
 
-    StatusCodes call(const QString &method, const QString &service, const QByteArray &args, QByteArray &ret) override;
+    StatusCode call(const QString &method, const QString &service, const QByteArray &args, QByteArray &ret) override;
     void call(const QString &method, const QString &service, const QByteArray &args, qtprotobuf::AsyncReply *reply) override;
     void subscribe(const QString &method, const QString &service, const QByteArray &args, AbstractClient *client, const std::function<void (const QByteArray &)> &handler) override;
 
@@ -51,7 +50,9 @@ protected:
 
 private:
     Q_DISABLE_COPY(Http2Channel)
-    Http2ChannelPrivate *d;
+
+    // PIMPL
+    struct Http2ChannelPrivate *d;
 };
 
 }

+ 36 - 30
tests/test_grpc/clienttest.cpp

@@ -88,10 +88,8 @@ TEST_F(ClientTest, StringEchoAsyncTest)
     QEventLoop waiter;
 
     AsyncReply *reply = testClient.testMethod(request);
-    QObject::connect(reply, &AsyncReply::finished, &m_app, [reply, &result, &waiter, &testClient]() {
-        if (testClient.lastError() == AbstractChannel::StatusCodes::Ok) {
-            result = reply->read<SimpleStringMessage>();
-        }
+    QObject::connect(reply, &AsyncReply::finished, &m_app, [reply, &result, &waiter]() {
+        result = reply->read<SimpleStringMessage>();
         waiter.quit();
     });
 
@@ -107,10 +105,8 @@ TEST_F(ClientTest, StringEchoAsync2Test)
     SimpleStringMessage request;
     request.setTestFieldString("Hello beach!");
     QEventLoop waiter;
-    testClient.testMethod(request, &m_app, [&result, &waiter, &testClient](AsyncReply *reply) {
-        if (testClient.lastError() == AbstractChannel::StatusCodes::Ok) {
-            result = reply->read<SimpleStringMessage>();
-        }
+    testClient.testMethod(request, &m_app, [&result, &waiter](AsyncReply *reply) {
+        result = reply->read<SimpleStringMessage>();
         waiter.quit();
     });
 
@@ -118,7 +114,7 @@ TEST_F(ClientTest, StringEchoAsync2Test)
     ASSERT_STREQ(result.testFieldString().toStdString().c_str(), "Hello beach!");
 }
 
-TEST_F(ClientTest, StringEchoAsyncAbortTest)
+TEST_F(ClientTest, StringEchoImmediateAsyncAbortTest)
 {
     TestServiceClient testClient;
     testClient.attachChannel(std::make_shared<Http2Channel>(m_echoServerAddress, InsecureCredentials()));
@@ -126,38 +122,52 @@ TEST_F(ClientTest, StringEchoAsyncAbortTest)
     SimpleStringMessage request;
     request.setTestFieldString("sleep");
     QEventLoop waiter;
-
-    bool errorCalled = false;
     AsyncReply *reply = testClient.testMethod(request);
+
     result.setTestFieldString("Result not changed by echo");
-    QObject::connect(reply, &AsyncReply::finished, &m_app, [reply, &result, &waiter, &testClient]() {
-        if (testClient.lastError() == AbstractChannel::StatusCodes::Ok) {
-            result = reply->read<SimpleStringMessage>();
-        }
+    QObject::connect(reply, &AsyncReply::finished, &m_app, [&waiter, &result, reply]() {
+        result = reply->read<SimpleStringMessage>();
         waiter.quit();
     });
 
-    QObject::connect(reply, &AsyncReply::error, reply, [&errorCalled](AbstractChannel::StatusCodes){
-        errorCalled = true;
+    AbstractChannel::StatusCode asyncStatus = AbstractChannel::StatusCode::Ok;
+    QObject::connect(reply, &AsyncReply::error, reply, [&asyncStatus](AbstractChannel::StatusCode code){
+        asyncStatus = code;
     });
+
+    AbstractChannel::StatusCode clientStatus = AbstractChannel::StatusCode::Ok;
+    QObject::connect(&testClient, &TestServiceClient::error, reply, [&clientStatus](AbstractChannel::StatusCode code, QString errMsg){
+        clientStatus = code;
+        std::cerr << code << ":" << errMsg.toStdString();
+    });
+
     QTimer::singleShot(5000, &waiter, &QEventLoop::quit);
     reply->abort();
-
     waiter.exec();
+
+    ASSERT_EQ(clientStatus, AbstractChannel::StatusCode::Aborted);
+    ASSERT_EQ(asyncStatus, AbstractChannel::StatusCode::Aborted);
     ASSERT_STREQ(result.testFieldString().toStdString().c_str(), "Result not changed by echo");
-    ASSERT_EQ(testClient.lastError(), AbstractChannel::StatusCodes::Aborted);
-    ASSERT_TRUE(errorCalled);
+}
 
+TEST_F(ClientTest, StringEchoDeferredAsyncAbortTest)
+{
+    TestServiceClient testClient;
+    testClient.attachChannel(std::make_shared<Http2Channel>(m_echoServerAddress, InsecureCredentials()));
+    SimpleStringMessage result;
+    SimpleStringMessage request;
+    request.setTestFieldString("sleep");
+    QEventLoop waiter;
+    AsyncReply *reply = testClient.testMethod(request);
 
-    errorCalled = false;
+    result.setTestFieldString("Result not changed by echo");
+    bool errorCalled = false;
     reply = testClient.testMethod(request);
-    QObject::connect(reply, &AsyncReply::finished, &m_app, [reply, &result, &waiter, &testClient]() {
-        if (testClient.lastError() == AbstractChannel::StatusCodes::Ok) {
-            result = reply->read<SimpleStringMessage>();
-        }
+    QObject::connect(reply, &AsyncReply::finished, &m_app, [reply, &result, &waiter]() {
+        result = reply->read<SimpleStringMessage>();
         waiter.quit();
     });
-    QObject::connect(reply, &AsyncReply::error, reply, [&errorCalled](AbstractChannel::StatusCodes){
+    QObject::connect(reply, &AsyncReply::error, reply, [&errorCalled](AbstractChannel::StatusCode){
         errorCalled = true;
     });
 
@@ -167,7 +177,6 @@ TEST_F(ClientTest, StringEchoAsyncAbortTest)
     waiter.exec();
 
     ASSERT_STREQ(result.testFieldString().toStdString().c_str(), "Result not changed by echo");
-    ASSERT_EQ(testClient.lastError(), AbstractChannel::StatusCodes::Aborted);
     ASSERT_TRUE(errorCalled);
 }
 
@@ -199,7 +208,6 @@ TEST_F(ClientTest, StringEchoStreamTest)
 
     ASSERT_EQ(i, 4);
     ASSERT_STREQ(result.testFieldString().toStdString().c_str(), "Stream1Stream2Stream3Stream4");
-    ASSERT_EQ(testClient.lastError(), AbstractChannel::StatusCodes::Ok);
 }
 
 TEST_F(ClientTest, StringEchoStreamTestRetUpdates)
@@ -225,7 +233,6 @@ TEST_F(ClientTest, StringEchoStreamTestRetUpdates)
 
     ASSERT_EQ(i, 4);
     ASSERT_STREQ(result->testFieldString().toStdString().c_str(), "Stream4");
-    ASSERT_EQ(testClient.lastError(), AbstractChannel::StatusCodes::Ok);
 }
 
 
@@ -254,5 +261,4 @@ TEST_F(ClientTest, HugeBlobEchoStreamTest)
 
     QByteArray returnDataHash = QCryptographicHash::hash(result.testBytes(), QCryptographicHash::Sha256);
     ASSERT_TRUE(returnDataHash == dataHash);
-    ASSERT_EQ(testClient.lastError(), AbstractChannel::StatusCodes::Ok);
 }