Fix bug : TCPClient will crash when we do 'Disconnect' and descrute t…

…he object immediately. see #17
Qihoo360 · Mar 24, 2017 · 05d91ff · 05d91ff
1 parent 72d55d9
commit 05d91ff
Show file tree

Hide file tree

Showing 7 changed files with 127 additions and 44 deletions.
diff --git a/evpp/tcp_client.cc b/evpp/tcp_client.cc
@@ -24,7 +24,13 @@ TCPClient::~TCPClient() {
     auto_reconnect_.store(false);
     TCPConnPtr c = conn();
     if (c) {
-        assert(c->IsDisconnected());
+        // Most of the cases, the conn_ is at disconnected status at this time.
+        // But some times, the user application layer will call TCPClient::Close()
+        // and delete TCPClient object immediately, that will make conn_ to be at disconnecting status.
+        assert(c->IsDisconnected() || c->IsDisconnecting());
+        if (c->IsDisconnecting()) {
+            c->SetCloseCallback(CloseCallback());
+        }
     }
     conn_.reset();
 }
@@ -58,6 +64,7 @@ void TCPClient::DisconnectInLoop() {
 
     if (conn_) {
         LOG_TRACE << "Close the TCPConn " << conn_.get() << " status=" << conn_->StatusToString();
+        assert(!conn_->IsDisconnected() && !conn_->IsDisconnecting());
         conn_->Close();
     } else {
         // When connector_ is connecting to the remote server ...

diff --git a/evpp/tcp_conn.cc b/evpp/tcp_conn.cc
@@ -49,6 +49,7 @@ TCPConn::~TCPConn() {
 
 void TCPConn::Close() {
     LOG_INFO << "TCPConn::Close this=" << this << " fd=" << fd_ << " status=" << StatusToString() << " addr=" << Addr();
+    status_ = kDisconnecting;
     auto c = shared_from_this();
     auto f = [c]() {
         assert(c->loop_->IsInLoopThread());
@@ -145,7 +146,7 @@ void TCPConn::SendInLoop(const void* data, size_t len) {
     }
 
     if (write_error) {
-        HandleClose();
+        HandleError();
         return;
     }
 
@@ -178,6 +179,7 @@ void TCPConn::HandleRead() {
         if (type() == kOutgoing) {
             // This is an outgoing connection, we own it and it's done. so close it
             LOG_DEBUG << "TCPConn::HandleRead this=" << this << " fd=" << fd_ << ". We read 0 bytes and close the socket.";
+            status_ = kDisconnecting;
             HandleClose();
         } else {
             // Fix the half-closing problem : https://github.com/chenshuo/muduo/pull/117
@@ -193,7 +195,7 @@ void TCPConn::HandleRead() {
             LOG_DEBUG << "TCPConn::HandleRead errno=" << serrno << " " << strerror(serrno);
         } else {
             LOG_DEBUG << "TCPConn::HandleRead errno=" << serrno << " " << strerror(serrno) << " We are closing this connection now.";
-            HandleClose();
+            HandleError();
         }
     }
 }
@@ -219,12 +221,16 @@ void TCPConn::HandleWrite() {
         if (EVUTIL_ERR_RW_RETRIABLE(serrno)) {
             LOG_WARN << "TCPConn::HandleWrite errno=" << serrno << " " << strerror(serrno);
         } else {
-            HandleClose();
+            HandleError();
         }
     }
 }
 
 void TCPConn::DelayClose() {
+    assert(loop_->IsInLoopThread());
+    LOG_INFO << "TCPConn::DelayClose this=" << this << " addr=" << Addr() << " fd=" << fd_ << " status_=" << StatusToString();
+    status_ = kDisconnecting;
+    assert(delay_close_timer_.get());
     delay_close_timer_.reset();
     HandleClose();
 }
@@ -237,7 +243,10 @@ void TCPConn::HandleClose() {
         return;
     }
 
-    assert(status_ == kConnected);
+    // We call HandleClose() from TCPConn's method, the status_ is kConnected
+    // But we call HandleClose() from out of TCPConn's method, the status_ is kDisconnecting
+    assert(status_ == kDisconnecting);
+
     status_ = kDisconnecting;
     assert(loop_->IsInLoopThread());
     chan_->DisableAllEvent();
@@ -259,11 +268,19 @@ void TCPConn::HandleClose() {
         conn_fn_(conn);
     }
 
-    close_fn_(conn);
+    if (close_fn_) {
+        close_fn_(conn);
+    }
     LOG_INFO << "TCPConn::HandleClose exit, this=" << this << " addr=" << Addr() << " fd=" << fd_ << " status_=" << StatusToString() << " use_count=" << conn.use_count();
     status_ = kDisconnected;
 }
 
+void TCPConn::HandleError() {
+    LOG_INFO << "TCPConn::HandleError this=" << this << " fd=" << fd_ << " status=" << StatusToString();
+    status_ = kDisconnecting;
+    HandleClose();
+}
+
 void TCPConn::OnAttachedToLoop() {
     assert(loop_->IsInLoopThread());
     status_ = kConnected;

diff --git a/evpp/tcp_conn.h b/evpp/tcp_conn.h
@@ -124,16 +124,16 @@ class EVPP_EXPORT TCPConn : public std::enable_shared_from_this<TCPConn> {
         close_fn_ = cb;
     }
     void OnAttachedToLoop();
-
+    std::string StatusToString() const;
 private:
     void HandleRead();
     void HandleWrite();
     void HandleClose();
     void DelayClose();
+    void HandleError();
     void SendInLoop(const Slice& message);
     void SendInLoop(const void* data, size_t len);
     void SendStringInLoop(const std::string& message);
-    std::string StatusToString() const;
     std::string Addr() const {
         if (IsIncommingConn()) {
             return "(" + remote_addr_ + "->" + local_addr_ + "(local))";

diff --git a/test/tcp_client_reconnect_test.cc b/test/tcp_client_reconnect_test.cc
@@ -86,39 +86,3 @@ TEST_UNIT(testTCPClientReconnect) {
 
 
 
-
-TEST_UNIT(testTCPClientConnectFailed) {
-    std::shared_ptr<evpp::EventLoop> loop(new evpp::EventLoop);
-    std::shared_ptr<evpp::TCPClient> client(new evpp::TCPClient(loop.get(), addr, "TCPPingPongClient"));
-    client->SetConnectionCallback([ &loop, &client ](const evpp::TCPConnPtr& conn) {
-        H_TEST_ASSERT(!conn->IsConnected());
-        client->Disconnect();
-        loop->Stop();
-    });
-    client->set_auto_reconnect(false);
-    client->Connect();
-    loop->Run();
-    client.reset();
-    loop.reset();
-    H_TEST_ASSERT(evpp::GetActiveEventCount() == 0);
-}
-
-TEST_UNIT(testTCPClientDisconnectImmediately) {
-    std::shared_ptr<evpp::EventLoop> loop(new evpp::EventLoop);
-    std::shared_ptr<evpp::TCPClient> client(new evpp::TCPClient(loop.get(), "cmake.org:80", "TCPPingPongClient"));
-    client->SetConnectionCallback([loop, client](const evpp::TCPConnPtr& conn) {
-        H_TEST_ASSERT(!conn->IsConnected());
-        H_TEST_ASSERT(!loop->IsRunning());
-        auto f = [loop]() { loop->Stop(); };
-        loop->RunAfter(300.0, f);
-    });
-    client->set_auto_reconnect(false);
-    client->Connect();
-    client->Disconnect();
-    loop->Run();
-    client.reset();
-    loop.reset();
-    H_TEST_ASSERT(evpp::GetActiveEventCount() == 0);
-}
-
-
diff --git a/test/tcp_client_test.cc b/test/tcp_client_test.cc
@@ -0,0 +1,91 @@
+#include "test_common.h"
+
+#include <evpp/exp.h>
+#include <evpp/libevent_headers.h>
+#include <evpp/libevent_watcher.h>
+#include <evpp/event_loop.h>
+#include <evpp/event_loop_thread.h>
+#include <evpp/tcp_server.h>
+#include <evpp/buffer.h>
+#include <evpp/tcp_conn.h>
+#include <evpp/tcp_client.h>
+
+TEST_UNIT(testTCPClientConnectFailed) {
+    std::shared_ptr<evpp::EventLoop> loop(new evpp::EventLoop);
+    std::shared_ptr<evpp::TCPClient> client(new evpp::TCPClient(loop.get(), "127.0.0.1:39723", "TCPPingPongClient"));
+    client->SetConnectionCallback([&loop, &client](const evpp::TCPConnPtr& conn) {
+        H_TEST_ASSERT(!conn->IsConnected());
+        client->Disconnect();
+        loop->Stop();
+    });
+    client->set_auto_reconnect(false);
+    client->Connect();
+    loop->Run();
+    client.reset();
+    loop.reset();
+    H_TEST_ASSERT(evpp::GetActiveEventCount() == 0);
+}
+
+TEST_UNIT(testTCPClientDisconnectImmediately) {
+    std::shared_ptr<evpp::EventLoop> loop(new evpp::EventLoop);
+    std::shared_ptr<evpp::TCPClient> client(new evpp::TCPClient(loop.get(), "cmake.org:80", "TCPPingPongClient"));
+    client->SetConnectionCallback([loop, client](const evpp::TCPConnPtr& conn) {
+        H_TEST_ASSERT(!conn->IsConnected());
+        H_TEST_ASSERT(!loop->IsRunning());
+        auto f = [loop]() { loop->Stop(); };
+        loop->RunAfter(300.0, f);
+    });
+    client->set_auto_reconnect(false);
+    client->Connect();
+    client->Disconnect();
+    loop->Run();
+    client.reset();
+    loop.reset();
+    H_TEST_ASSERT(evpp::GetActiveEventCount() == 0);
+}
+
+
+
+namespace {
+    struct NSQConn {
+        NSQConn(evpp::EventLoop* loop) : loop_(loop) {
+            client_ = std::make_shared<evpp::TCPClient>(loop, "www.so.com:80", "TCPPingPongClient");
+            client_->SetConnectionCallback([=](const evpp::TCPConnPtr& conn) {
+                H_TEST_ASSERT(conn->IsConnected());
+                H_TEST_ASSERT(loop->IsRunning());
+                this->connected_ = true;
+                client_->SetConnectionCallback(evpp::ConnectionCallback());
+            });
+            client_->Connect();
+        }
+
+        void Disconnect() {
+            if (!connected_) {
+                loop_->RunAfter(100.0, [this]() {this->Disconnect(); });
+                return;
+            }
+
+            // We call TCPClient::Disconnect and then delete the hold object of TCPClient immediately
+            client_->Disconnect();
+            client_.reset();
+            connected_ = false;
+            loop_->RunAfter(100.0, [this]() {this->loop_->Stop(); });
+        }
+
+        std::shared_ptr<evpp::TCPClient> client_;
+        bool connected_ = false;
+        evpp::EventLoop* loop_;
+    };
+}
+
+TEST_UNIT(testTCPClientDisconnectAndDestruct) {
+    std::shared_ptr<evpp::EventLoop> loop(new evpp::EventLoop);
+    NSQConn nc(loop.get());
+    loop->RunAfter(100.0, [&nc]() {nc.Disconnect(); });
+    loop->Run();
+    loop.reset();
+    H_TEST_ASSERT(evpp::GetActiveEventCount() == 0);
+}
+
+
+
diff --git a/vsprojects/libevpp-test.vcxproj b/vsprojects/libevpp-test.vcxproj
@@ -130,6 +130,7 @@
       <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">false</ExcludedFromBuild>
       <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">false</ExcludedFromBuild>
     </ClCompile>
+    <ClCompile Include="..\test\tcp_client_test.cc" />
     <ClCompile Include="..\test\tcp_server_test.cc">
       <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">false</ExcludedFromBuild>
       <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">false</ExcludedFromBuild>

diff --git a/vsprojects/libevpp-test.vcxproj.filters b/vsprojects/libevpp-test.vcxproj.filters
@@ -90,6 +90,9 @@
     <ClCompile Include="..\test\dns_resolver_test.cc">
       <Filter>tcp</Filter>
     </ClCompile>
+    <ClCompile Include="..\test\tcp_client_test.cc">
+      <Filter>tcp</Filter>
+    </ClCompile>
   </ItemGroup>
   <ItemGroup>
     <ClInclude Include="..\test\test_common.h">