[pulsar] branch master updated: [C++] ClientImpl::handleClose shouldn't use statics (#7068)

[pe...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

penghui pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pulsar.git


The following commit(s) were added to refs/heads/master by this push:
     new ff594b8  [C++] ClientImpl::handleClose shouldn't use statics (#7068)
ff594b8 is described below

commit ff594b8144ace2ea8534773ff78ddb9d4a99455c
Author: Matteo Merli <mm...@apache.org>
AuthorDate: Thu May 28 20:21:24 2020 -0700

    [C++] ClientImpl::handleClose shouldn't use statics (#7068)
    
    ClientImpl::handleClose was using static variables to record the first
    error on closing a client. This is just wrong. A static stack variable
    in c++ acts like a global. So if errorClosing was ever set to true in
    a process, all clients closed in that process after that point would
    be errored with first error.
    
    This fixes a failure in BasicEndToEndTest.testDelayedMessages, which
    happens sporadically on C++, and always when running the tests in
    serial, probably due to a double close in some other test.
    
    Co-authored-by: Ivan Kelly <ik...@splunk.com>
---
 pulsar-client-cpp/lib/ClientImpl.cc | 24 +++++++++++-------------
 pulsar-client-cpp/lib/ClientImpl.h  |  4 +++-
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/pulsar-client-cpp/lib/ClientImpl.cc b/pulsar-client-cpp/lib/ClientImpl.cc
index 10db13b..cb9bf67 100644
--- a/pulsar-client-cpp/lib/ClientImpl.cc
+++ b/pulsar-client-cpp/lib/ClientImpl.cc
@@ -94,7 +94,8 @@ ClientImpl::ClientImpl(const std::string& serviceUrl, const ClientConfiguration&
       pool_(clientConfiguration_, ioExecutorProvider_, clientConfiguration_.getAuthPtr(), poolConnections),
       producerIdGenerator_(0),
       consumerIdGenerator_(0),
-      requestIdGenerator_(0) {
+      requestIdGenerator_(0),
+      closingError(ResultOk) {
     if (clientConfiguration_.getLogger()) {
         // A logger factory was explicitely configured. Let's just use that
         LogUtils::setLoggerFactory(clientConfiguration_.getLogger());
@@ -510,12 +511,12 @@ void ClientImpl::closeAsync(CloseCallback callback) {
 }
 
 void ClientImpl::handleClose(Result result, SharedInt numberOfOpenHandlers, ResultCallback callback) {
-    static bool errorClosing = false;
-    static Result failResult = ResultOk;
-    if (result != ResultOk) {
-        errorClosing = true;
-        failResult = result;
+    Result expected = ResultOk;
+    if (!closingError.compare_exchange_strong(expected, result)) {
+        LOG_DEBUG("Tried to updated closingError, but already set to "
+                  << expected << ". This means multiple errors have occurred while closing the client");
     }
+
     if (*numberOfOpenHandlers > 0) {
         --(*numberOfOpenHandlers);
     }
@@ -523,17 +524,14 @@ void ClientImpl::handleClose(Result result, SharedInt numberOfOpenHandlers, Resu
         Lock lock(mutex_);
         state_ = Closed;
         lock.unlock();
-        if (errorClosing) {
-            LOG_DEBUG("Problem in closing client, could not close one or more consumers or producers");
-            if (callback) {
-                callback(failResult);
-            }
-        }
 
         LOG_DEBUG("Shutting down producers and consumers for client");
         shutdown();
         if (callback) {
-            callback(ResultOk);
+            if (closingError != ResultOk) {
+                LOG_DEBUG("Problem in closing client, could not close one or more consumers or producers");
+            }
+            callback(closingError);
         }
     }
 }
diff --git a/pulsar-client-cpp/lib/ClientImpl.h b/pulsar-client-cpp/lib/ClientImpl.h
index 46b7c2b..46d713c 100644
--- a/pulsar-client-cpp/lib/ClientImpl.h
+++ b/pulsar-client-cpp/lib/ClientImpl.h
@@ -28,7 +28,7 @@
 #include <lib/TopicName.h>
 #include "ProducerImplBase.h"
 #include "ConsumerImplBase.h"
-
+#include <atomic>
 #include <vector>
 
 namespace pulsar {
@@ -148,6 +148,8 @@ class ClientImpl : public std::enable_shared_from_this<ClientImpl> {
     typedef std::vector<ConsumerImplBaseWeakPtr> ConsumersList;
     ConsumersList consumers_;
 
+    std::atomic<Result> closingError;
+
     friend class Client;
 };
 } /* namespace pulsar */