Repository: pinpoint-apm/pinpoint-c-agent (language: python)

Issue:
trace leak when load testing
**Describe the bug**

I found many leaked node, the traceId reached 10000.

If the parent trace node end before the child node, the child node would be leak




Discussion / hints:

Fix patch:
diff --git a/common/CMakeLists.txt b/common/CMakeLists.txt
index b3aefec92..444cdfadc 100644
--- a/common/CMakeLists.txt
+++ b/common/CMakeLists.txt
@@ -1,5 +1,5 @@
 cmake_minimum_required(VERSION 3.13)
-project(common VERSION 0.4.10 DESCRIPTION  "pinpoint common library")
+project(common VERSION 0.4.11 DESCRIPTION  "pinpoint common library")
 
 set(CMAKE_CXX_STANDARD 11)
 
diff --git a/common/include/common.h.in b/common/include/common.h.in
index ada167245..51f07ff06 100644
--- a/common/include/common.h.in
+++ b/common/include/common.h.in
@@ -281,6 +281,7 @@ extern "C"
      */
     void reset_unique_id(void);
     const char *pinpoint_agent_version();
+    void show_status(void);
 #ifdef __cplusplus
 }
 #endif
diff --git a/common/src/ConnectionPool/SpanConnectionPool.cpp b/common/src/ConnectionPool/SpanConnectionPool.cpp
index 3ec367fa8..5898fbaeb 100644
--- a/common/src/ConnectionPool/SpanConnectionPool.cpp
+++ b/common/src/ConnectionPool/SpanConnectionPool.cpp
@@ -1,12 +1,12 @@
 ////////////////////////////////////////////////////////////////////////////////
 // Copyright 2020 NAVER Corp
-// 
+//
 // Licensed under the Apache License, Version 2.0 (the "License"); you may not
 // use this file except in compliance with the License.  You may obtain a copy
 // of the License at
-// 
+//
 //   http://www.apache.org/licenses/LICENSE-2.0
-// 
+//
 // Unless required by applicable law or agreed to in writing, software
 // distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
 // WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
@@ -22,90 +22,86 @@
 
 #include "SpanConnectionPool.h"
 #include "Cache/SafeSharedState.h"
-#include <sstream> 
+#include <sstream>
 
-namespace ConnectionPool {
-using Cache::SafeSharedState;
-namespace Json = AliasJson;
-SpanConnectionPool::SpanConnectionPool(const char* co_host):
-        co_host(co_host),
-        con_counter(0)
+namespace ConnectionPool
 {
-    this->_cPool.push(this->createTrans());
-}
-
-
-TransConnection SpanConnectionPool::createTrans()
-{
-    TransConnection _connect( new TransLayer(this->co_host));
-    using namespace std::placeholders;
-    // _connect->registerPeerMsgCallback(
-    //     std::bind(&SpanConnectionPool::_handleMsgFromCollector,this,_1,_2,_3),
-    //     std::bind(&SpanConnectionPool::_handleTransLayerState,this,_1));
-
-    _connect->registerPeerMsgCallback(
-        std::bind(&SpanConnectionPool::_handleMsgFromCollector,this,_1,_2,_3),
-        nullptr);
-    this->con_counter++;
-
-    return _connect;
-}
-
-//@obsoleted
-// void SpanConnectionPool::_handleTransLayerState(int state)
-// {
-//     if(state == E_OFFLINE)
-//     {
-//         if(global_agent_info.inter_flag & E_UTEST){
-//             SafeSharedState::instance().markONLine();
-//         }
-//         else{
-//             SafeSharedState::instance().markOFFLine();
-//         }
-//     }
-// }
-
-
-
-void SpanConnectionPool::_handleMsgFromCollector(int type,const char* buf,size_t len)
-{
-    switch (type)
+    using Cache::SafeSharedState;
+    namespace Json = AliasJson;
+    SpanConnectionPool::SpanConnectionPool(const char *co_host) : co_host(co_host),
+                                                                  con_counter(0)
     {
+        this->_cPool.push(this->createTrans());
+    }
 
-    case RESPONSE_AGENT_INFO:
-        this->_handle_agent_info(type,buf,len);
-        break;
-    
-    default:
-        break;
+    TransConnection SpanConnectionPool::createTrans()
+    {
+        TransConnection _connect(new TransLayer(this->co_host));
+        using namespace std::placeholders;
+        // _connect->registerPeerMsgCallback(
+        //     std::bind(&SpanConnectionPool::_handleMsgFromCollector,this,_1,_2,_3),
+        //     std::bind(&SpanConnectionPool::_handleTransLayerState,this,_1));
+
+        _connect->registerPeerMsgCallback(
+            std::bind(&SpanConnectionPool::_handleMsgFromCollector, this, _1, _2, _3),
+            nullptr);
+        this->con_counter++;
+
+        return _connect;
     }
 
-}
+    //@obsoleted
+    // void SpanConnectionPool::_handleTransLayerState(int state)
+    // {
+    //     if(state == E_OFFLINE)
+    //     {
+    //         if(global_agent_info.inter_flag & E_UTEST){
+    //             SafeSharedState::instance().markONLine();
+    //         }
+    //         else{
+    //             SafeSharedState::instance().markOFFLine();
+    //         }
+    //     }
+    // }
+
+    void SpanConnectionPool::_handleMsgFromCollector(int type, const char *buf, size_t len)
+    {
+        switch (type)
+        {
 
-void SpanConnectionPool::_handle_agent_info(int type,const char* buf,size_t len)
-{
-    Json::Value  root;
-    (void)type;
-    Json::CharReaderBuilder builder;
-    builder["collectComments"] = false;
+        case RESPONSE_AGENT_INFO:
+            this->_handle_agent_info(type, buf, len);
+            break;
 
-    std::istringstream inss(std::string(buf,len));
-    JSONCPP_STRING errs;
-    bool ok = parseFromStream(builder, inss, &root, &errs);
+        default:
+            break;
+        }
+    }
 
-    if(ok)
+    void SpanConnectionPool::_handle_agent_info(int type, const char *buf, size_t len)
     {
-        // pp_trace("collector-agent say:%s",root.toStyledString().c_str());
-
-        if(root.isMember("time")){
-            SafeSharedState::instance().updateStartTime(atoll(root["time"].asCString()));
+        Json::Value root;
+        (void)type;
+        Json::CharReaderBuilder builder;
+        builder["collectComments"] = false;
+
+        std::istringstream inss(std::string(buf, len));
+        JSONCPP_STRING errs;
+        bool ok = parseFromStream(builder, inss, &root, &errs);
+
+        if (ok)
+        {
+            // pp_trace("collector-agent say:%s",root.toStyledString().c_str());
+
+            if (root.isMember("time"))
+            {
+                SafeSharedState::instance().updateStartTime(atoll(root["time"].asCString()));
+            }
+        }
+        else
+        {
+            pp_trace("Recieve invalid msg: %.*s from Collector-agent, reason: %s", (int)len, buf, errs.c_str());
         }
-        
-    }else{
-        pp_trace("Recieve invalid msg: %.*s from Collector-agent, reason: %s",len,buf,errs.c_str());
     }
 
-}
-
-
 } /* namespace ConnectionPool */
diff --git a/common/src/NodePool/PoolManager.h b/common/src/NodePool/PoolManager.h
index 973889625..a5479833f 100644
--- a/common/src/NodePool/PoolManager.h
+++ b/common/src/NodePool/PoolManager.h
@@ -56,6 +56,16 @@ namespace NodePool
             return this->_freeNodeList.size();
         }
 
+        void foreachAliveNode(std::function<void(int i)> func)
+        {
+            std::lock_guard<std::mutex> _safe(this->_lock);
+            for (int32_t id = 0; id < this->maxId; id++)
+            {
+                if (this->nodeIsAlive(id))
+                    func(id + 1);
+            }
+        }
+
 #ifdef COMMON_DEBUG
         inline bool NoNodeLeak()
         {
diff --git a/common/src/NodePool/TraceNode.cpp b/common/src/NodePool/TraceNode.cpp
index bd041baac..ce4f740d9 100644
--- a/common/src/NodePool/TraceNode.cpp
+++ b/common/src/NodePool/TraceNode.cpp
@@ -82,14 +82,15 @@ namespace NodePool
     void TraceNode::convertToSpanEvent()
     {
         this->setNodeValue("E", this->cumulative_time);
-        TraceNode &root = PoolManager::getInstance().GetNode(this->mRootId);
-        this->setNodeValue("S", this->start_time - root.start_time);
+        this->setNodeValue("S", this->start_time - this->root_start_time);
     }
 
     void TraceNode::setTraceParent(TraceNode &parent)
     {
         this->startParentId = parent.ID;
         this->mRootId = parent.mRootId;
+        TraceNode &root = PoolManager::getInstance().GetNode(this->mRootId);
+        this->root_start_time = root.start_time;
     }
 
     void TraceNode::startTimer()
diff --git a/common/src/NodePool/TraceNode.h b/common/src/NodePool/TraceNode.h
index efa754ead..4306ef5cf 100644
--- a/common/src/NodePool/TraceNode.h
+++ b/common/src/NodePool/TraceNode.h
@@ -57,6 +57,7 @@ namespace NodePool
         uint64_t fetal_error_time;
         uint64_t limit;
         uint64_t cumulative_time;
+        uint64_t root_start_time;
         bool mHasExp;
 
     public:
diff --git a/common/src/common.cpp b/common/src/common.cpp
index 5d9991142..618dfda9b 100644
--- a/common/src/common.cpp
+++ b/common/src/common.cpp
@@ -169,8 +169,16 @@ NodeID do_end_trace(NodeID Id)
         // check opt
         if (node.checkOpt() == true)
         {
-            TraceNode &parent = PoolManager::getInstance().GetNode(parentId);
-            parent.addChild(node);
+            try
+            {
+                TraceNode &parent = PoolManager::getInstance().GetNode(parentId);
+                parent.addChild(node);
+            }
+            catch (const std::out_of_range &ex)
+            {
+                pp_trace("current#%d dropped,due to parent is end", node.ID);
+                PoolManager::getInstance().freeNode(node);
+            }
         }
         else
         {
@@ -303,13 +311,13 @@ NodeID pinpoint_end_trace(NodeID traceId)
         pp_trace("#%d pinpoint_end_trace Done!", traceId);
         return ret;
     }
-    catch (const std::out_of_range &)
+    catch (const std::out_of_range &ex)
     {
-        pp_trace("#%d not found", traceId);
+        pp_trace("end_trace %d out_of_range exception: %s", traceId, ex.what());
     }
     catch (const std::exception &ex)
     {
-        pp_trace("#%d end trace failed. %s", traceId, ex.what());
+        pp_trace("end_trace #%d end trace failed. %s", traceId, ex.what());
     }
 
     return E_INVALID_NODE;
@@ -678,4 +686,19 @@ void register_span_handler(void (*handler)(const char *))
     {
         _SpanHandler_ = std::bind(handler, std::placeholders::_1);
     }
+}
+
+void show_status(void)
+{
+    Json::Value status;
+    status["pool_total_node"] = PoolManager::getInstance().totoalNodesCount();
+    status["pool_free_node"] = PoolManager::getInstance().freeNodesCount();
+
+    auto add_alive_node_fun = [&status](int _id)
+    {
+        status["pool_alive_nodes"].append(_id);
+    };
+    PoolManager::getInstance().foreachAliveNode(std::bind(add_alive_node_fun, std::placeholders::_1));
+
+    fprintf(stderr, "%s\n", status.toStyledString().c_str());
 }
\ No newline at end of file


Test patch:
diff --git a/common/test/test_node.cc b/common/test/test_node.cc
index aae2857f8..718158888 100644
--- a/common/test/test_node.cc
+++ b/common/test/test_node.cc
@@ -235,4 +235,21 @@ TEST(node, pinpoint_start_traceV1)
     EXPECT_TRUE(span.find("Exception") != span.npos);
     EXPECT_TRUE(span.find("TraceMinTimeMs:2000") == span.npos);
     EXPECT_TRUE(span.find("NoException") == span.npos);
+}
+
+TEST(node, leak_node)
+{
+    auto count = PoolManager::getInstance().freeNodesCount();
+    NodeID root, child1, child2;
+    root = pinpoint_start_trace(E_ROOT_NODE);
+    child1 = pinpoint_start_trace(root);
+    child2 = pinpoint_start_trace(child1);
+    // root goes first
+    pinpoint_end_trace(root);
+
+    pinpoint_end_trace(child1);
+    pinpoint_end_trace(child2);
+
+    EXPECT_EQ(count, PoolManager::getInstance().freeNodesCount());
+    show_status();
 }
\ No newline at end of file
diff --git a/src/PHP/tests/node_test.phpt b/src/PHP/tests/node_test.phpt
index 813f50936..e281c87d7 100644
--- a/src/PHP/tests/node_test.phpt
+++ b/src/PHP/tests/node_test.phpt
@@ -32,4 +32,4 @@ pinpoint_end_trace($unexist_node);
 [pinpoint] [%d] [%d] pinpoint_add_clues#1024 failed.Reason #1024 is not alive,parameters:32:fdfd
 [pinpoint] [%d] [%d] pinpoint_add_clues#1024 failed.Reason #1024 is not alive,parameters:32:fdfd
 [pinpoint] [%d] [%d] pinpoint_get_context_key#1024 failed with #1024 is not alive, parameters:b
-[pinpoint] [%d] [%d]#1024 not found
\ No newline at end of file
+[pinpoint] [%d] [%d]end_trace 1024 out_of_range exception: #1024 is not alive
\ No newline at end of file
diff --git a/src/PHP/tests5/node_test.phpt b/src/PHP/tests5/node_test.phpt
index 813f50936..e281c87d7 100644
--- a/src/PHP/tests5/node_test.phpt
+++ b/src/PHP/tests5/node_test.phpt
@@ -32,4 +32,4 @@ pinpoint_end_trace($unexist_node);
 [pinpoint] [%d] [%d] pinpoint_add_clues#1024 failed.Reason #1024 is not alive,parameters:32:fdfd
 [pinpoint] [%d] [%d] pinpoint_add_clues#1024 failed.Reason #1024 is not alive,parameters:32:fdfd
 [pinpoint] [%d] [%d] pinpoint_get_context_key#1024 failed with #1024 is not alive, parameters:b
-[pinpoint] [%d] [%d]#1024 not found
\ No newline at end of file
+[pinpoint] [%d] [%d]end_trace 1024 out_of_range exception: #1024 is not alive
\ No newline at end of file