Bugfix: SignalTrace mode has memory and deadlock issues (#3019)

* Bugfix: SignalTrace mode has memory and deadlock issues

* Bugfix: Memory leak of SignalSync and wrong status of global priority bthread

Author: chenBright

.github/actions/init-ut-make-config/action.yml

@@ -7,6 +7,8 @@ runs:
   steps:
   - run: |
       sudo apt-get update && sudo apt-get install -y clang-12 lldb-12 lld-12 libgtest-dev cmake gdb libstdc++6-11-dbg
+    shell: bash
+  - run: |
       cd /usr/src/gtest && export CC=clang-12 && export CXX=clang++-12 && sudo cmake -DCMAKE_POLICY_VERSION_MINIMUM=3.5 .
       sudo make -j ${{env.proc_num}} && sudo mv lib/libgtest* /usr/lib/
     shell: bash
@@ -22,5 +24,11 @@ runs:
       sudo ./autogen.sh && sudo CC=clang-12 CXX=clang++-12 ./configure --prefix=/gperftools --enable-frame-pointers 
       sudo make -j ${{env.proc_num}} && sudo make install
     shell: bash
-  - run: sh config_brpc.sh --headers="/libunwind/include /gperftools/include /usr/include" --libs="/libunwind/lib /gperftools/lib /usr/lib /usr/lib64" ${{inputs.options}}
+  - run: |
+      sudo git clone https://github.com/abseil/abseil-cpp.git 
+      cd abseil-cpp && sudo git checkout lts_2022_06_23 && sudo mkdir -p /abseil-cpp 
+      sudo CC=clang-12 CXX=clang++-12 cmake -DBUILD_TESTING=OFF -DCMAKE_POSITION_INDEPENDENT_CODE=ON -DCMAKE_INSTALL_PREFIX=/abseil-cpp -DCMAKE_POLICY_VERSION_MINIMUM=3.5 . 
+      sudo make -j ${{env.proc_num}} && sudo sudo make install
+    shell: bash
+  - run: sh config_brpc.sh --headers="/libunwind/include /gperftools/include /abseil-cpp/include /usr/include" --libs="/libunwind/lib /gperftools/lib /abseil-cpp/lib /usr/lib /usr/lib64" --cc=clang-12 --cxx=clang++-12 ${{inputs.options}} && cat config.mk
     shell: bash

.github/actions/install-all-dependencies/action.yml

@@ -4,7 +4,13 @@ runs:
     - uses: ./.github/actions/install-essential-dependencies
     - run: sudo apt-get install -y libunwind-dev libgoogle-glog-dev automake bison flex libboost-all-dev libevent-dev libtool pkg-config  libibverbs1 libibverbs-dev
       shell: bash
-    - run: wget https://archive.apache.org/dist/thrift/0.11.0/thrift-0.11.0.tar.gz && tar -xf thrift-0.11.0.tar.gz
+    - run: | 
+        wget https://archive.apache.org/dist/thrift/0.11.0/thrift-0.11.0.tar.gz && tar -xf thrift-0.11.0.tar.gz && cd thrift-0.11.0/
+        ./configure --prefix=/usr --with-rs=no --with-ruby=no --with-python=no --with-java=no --with-go=no --with-perl=no --with-php=no --with-csharp=no --with-erlang=no --with-lua=no --with-nodejs=no --with-haskell=no --with-dotnetcore=no CXXFLAGS="-Wno-unused-variable"
+        make -j ${{env.proc_num}} && sudo make install
       shell: bash
-    - run: cd thrift-0.11.0/ && ./configure --prefix=/usr --with-rs=no --with-ruby=no --with-python=no --with-java=no --with-go=no --with-perl=no --with-php=no --with-csharp=no --with-erlang=no --with-lua=no --with-nodejs=no --with-haskell=no --with-dotnetcore=no CXXFLAGS="-Wno-unused-variable" && make -j ${{env.proc_num}} && sudo make install
+    - run: |
+        git clone https://github.com/abseil/abseil-cpp.git && cd abseil-cpp && git checkout lts_2022_06_23
+        cmake -DBUILD_TESTING=OFF -DCMAKE_POSITION_INDEPENDENT_CODE=ON -DCMAKE_INSTALL_PREFIX=/usr -DCMAKE_POLICY_VERSION_MINIMUM=3.5 .
+        make -j ${{env.proc_num}} && sudo make install
       shell: bash

.github/workflows/ci-linux.yml

@@ -176,7 +176,7 @@ jobs:
     - uses: ./.github/actions/install-essential-dependencies
     - uses: ./.github/actions/init-ut-make-config
       with:
-        options: --cc=clang-12 --cxx=clang++-12 --with-bthread-tracer
+        options: --with-bthread-tracer
     - name: compile tests
       run: |
         cat config.mk
@@ -194,7 +194,7 @@ jobs:
     - uses: ./.github/actions/install-essential-dependencies
     - uses: ./.github/actions/init-ut-make-config
       with:
-        options: --cc=clang-12 --cxx=clang++-12 --with-bthread-tracer --with-asan
+        options: --with-bthread-tracer --with-asan
     - name: compile tests
       run: |
         cat config.mk

BUILD.bazel

@@ -50,9 +50,6 @@ COPTS = [
 }) + select({
     "//bazel/config:brpc_with_debug_lock": ["-DBRPC_DEBUG_LOCK=1"],
     "//conditions:default": ["-DBRPC_DEBUG_LOCK=0"],
-}) + select({
-    "//bazel/config:brpc_with_bthread_tracer": ["-DBRPC_BTHREAD_TRACER"],
-    "//conditions:default": [],
 }) + select({
     "//bazel/config:brpc_with_asan": ["-fsanitize=address"],
     "//conditions:default": [""],
@@ -402,6 +399,12 @@ cc_library(
         "src/bthread/*.h",
         "src/bthread/*.list",
     ]),
+    defines = [] + select({
+        "//bazel/config:brpc_with_bthread_tracer": [
+            "-DBRPC_BTHREAD_TRACER",
+        ],
+        "//conditions:default": [],
+    }),
     copts = COPTS,
     includes = [
         "src/",
@@ -412,7 +415,11 @@ cc_library(
         ":butil",
         ":bvar",
     ] + select({
-        "//bazel/config:brpc_with_bthread_tracer": ["@com_github_libunwind_libunwind//:libunwind"],
+        "//bazel/config:brpc_with_bthread_tracer": [
+            "@com_github_libunwind_libunwind//:libunwind",
+            "@com_google_absl//absl/debugging:stacktrace",
+            "@com_google_absl//absl/debugging:symbolize",
+        ],
         "//conditions:default": [],
     }),
 )

CMakeLists.txt

@@ -93,6 +93,8 @@ if (WITH_BTHREAD_TRACER)
     if (NOT LIBUNWIND_INCLUDE_PATH OR NOT LIBUNWIND_LIB)
         message(FATAL_ERROR "Fail to find libunwind, which is needed by bthread tracer")
     endif()
+    find_package(absl REQUIRED CONFIG)
+    set(bthread_tracer_ABSL_USED_TARGETS absl::base absl::stacktrace absl::symbolize)
     add_definitions(-DBRPC_BTHREAD_TRACER)
     include_directories(${LIBUNWIND_INCLUDE_PATH})
 endif ()
@@ -329,8 +331,8 @@ if(WITH_SNAPPY)
 endif()
 
 if (WITH_BTHREAD_TRACER)
-    set(DYNAMIC_LIB ${DYNAMIC_LIB} ${LIBUNWIND_LIB} ${LIBUNWIND_X86_64_LIB})
-    set(BRPC_PRIVATE_LIBS "${BRPC_PRIVATE_LIBS} -lunwind -lunwind-x86_64")
+    set(DYNAMIC_LIB ${DYNAMIC_LIB} ${LIBUNWIND_LIB} ${LIBUNWIND_X86_64_LIB} ${bthread_tracer_ABSL_USED_TARGETS})
+    set(BRPC_PRIVATE_LIBS "${BRPC_PRIVATE_LIBS} -lunwind -lunwind-x86_64  -labsl_stacktrace -labsl_symbolize -labsl_debugging_internal -labsl_demangle_internal -labsl_malloc_internal -labsl_raw_logging_internal -labsl_spinlock_wait -labsl_base")
 endif()
 
 if(CMAKE_SYSTEM_NAME STREQUAL "Linux")

MODULE.bazel

@@ -16,7 +16,7 @@ bazel_dep(name = "apple_support", version = "1.17.1")
 bazel_dep(name = 'rules_cc', version = '0.0.1')
 bazel_dep(name = 'rules_proto', version = '4.0.0')
 bazel_dep(name = 'zlib', version = '1.3.1.bcr.5', repo_name = 'com_github_madler_zlib')
-bazel_dep(name = "libunwind", version = "1.8.1", repo_name = 'com_github_libunwind_libunwind')
+bazel_dep(name = 'libunwind', version = '1.8.1', repo_name = 'com_github_libunwind_libunwind')
 
 # --registry=https://baidu.github.io/babylon/registry
 bazel_dep(name = 'leveldb', version = '1.23', repo_name = 'com_github_google_leveldb')

config_brpc.sh

@@ -363,6 +363,8 @@ if [ $WITH_BTHREAD_TRACER != 0 ]; then
     fi
     LIBUNWIND_HDR=$(find_dir_of_header_or_die libunwind.h)
     LIBUNWIND_LIB=$(find_dir_of_lib_or_die unwind)
+    ABSL_HDR=$(find_dir_of_header_or_die absl/base/config.h)
+    ABSL_LIB=$(find_dir_of_lib_or_die absl_symbolize)
 
     CPPFLAGS="${CPPFLAGS} -DBRPC_BTHREAD_TRACER"
 
@@ -371,6 +373,11 @@ if [ $WITH_BTHREAD_TRACER != 0 ]; then
     else
         STATIC_LINKINGS="$STATIC_LINKINGS -lunwind -lunwind-x86_64"
     fi
+    if [ -f "$ABSL_LIB/libabsl_base.$SO" ]; then
+        DYNAMIC_LINKINGS="$DYNAMIC_LINKINGS -labsl_stacktrace -labsl_symbolize -labsl_debugging_internal -labsl_demangle_internal -labsl_malloc_internal -labsl_raw_logging_internal -labsl_spinlock_wait -labsl_base"
+    else
+        STATIC_LINKINGS="$STATIC_LINKINGS -labsl_stacktrace -labsl_symbolize -labsl_debugging_internal -labsl_demangle_internal -labsl_malloc_internal -labsl_raw_logging_internal -labsl_spinlock_wait -labsl_base"
+    fi
 fi
 
 HDRS=$($ECHO "$LIBUNWIND_HDR\n$GFLAGS_HDR\n$PROTOBUF_HDR\n$ABSL_HDR\n$LEVELDB_HDR\n$OPENSSL_HDR" | sort | uniq)

docs/cn/bthread_tracer.md

@@ -58,8 +58,8 @@ jump_stack是bthread挂起或者运行的必经之路，也是STB的拦截点。
 
 # 使用方法
 
-1. 下载安装libunwind。
-2. 给config_brpc.sh增加`--with-bthread-tracer`选项或者给cmake增加`-DWITH_BTHREAD_TRACER=ON`选项。
+1. 下载安装libunwind和abseil-cpp。
+2. 给config_brpc.sh增加`--with-bthread-tracer`选项或者给cmake增加`-DWITH_BTHREAD_TRACER=ON`选项或者给bazel（Bzlmod模式）增加`--define with_bthread_tracer=true`选项。
 3. 访问服务的内置服务：`http://ip:port/bthreads/<bthread_id>?st=1`或者代码里调用`bthread::stack_trace()`函数。
 4. 如果希望追踪pthread的调用栈，在对应pthread上调用`bthread::init_for_pthread_stack_trace()`函数获取一个伪bthread_t，然后使用步骤3即可获取pthread调用栈。
 
@@ -75,5 +75,4 @@ jump_stack是bthread挂起或者运行的必经之路，也是STB的拦截点。
 
 # 相关flag
 
-- `enable_fast_unwind`：是否启用快速回溯功能，默认为true。大多数情况下，不需要关闭快速回溯功能。除非你关注的调用栈函数名转换失败，显示为`<unknown>`，则可以尝试关闭快速回溯功能，但这会导致性能下降。以包含30帧的调用栈举例，快速回溯基本上在200us以内就可以完成，而关闭快速回溯则需要4ms左右，性能下降了近20倍。
-- `signal_trace_timeout_ms`：信号追踪模式的超时时间，默认为50ms。虽然libunwind文档显示回溯功能是异步信号安全的，但是[gpertools社区发现libunwind在某些情况下会死锁](https://github.com/gperftools/gperftools/issues/775)，所以TaskTracer会设置了超时时间，超时后会放弃回溯，打破死锁。
+- `signal_trace_timeout_ms`：信号追踪模式的超时时间，默认为50ms。

src/bthread/task_control.cpp

@@ -209,7 +209,7 @@ int TaskControl::init(int concurrency) {
             "bthread_worker_usage", tag_str, _tagged_cumulated_worker_time[i], 1));
         _tagged_nbthreads.push_back(new bvar::Adder<int64_t>("bthread_count", tag_str));
         if (_priority_queues[i].init(BTHREAD_MAX_CONCURRENCY) != 0) {
-            LOG(FATAL) << "Fail to init _priority_q";
+            LOG(ERROR) << "Fail to init _priority_q";
             return -1;
         }
     }

src/bthread/task_group.cpp

@@ -496,7 +496,7 @@ int TaskGroup::start_foreground(TaskGroup** pg,
             (bool)(using_attr.flags & BTHREAD_NOSIGNAL)
         };
         g->set_remained(fn, &args);
-        TaskGroup::sched_to(pg, m->tid);
+        sched_to(pg, m->tid);
     }
     return 0;
 }
@@ -870,6 +870,10 @@ void TaskGroup::ready_to_run_in_worker_ignoresignal(void* args_in) {
 
 void TaskGroup::priority_to_run(void* args_in) {
     ReadyToRunArgs* args = static_cast<ReadyToRunArgs*>(args_in);
+#ifdef BRPC_BTHREAD_TRACER
+    tls_task_group->_control->_task_tracer.set_status(
+        TASK_STATUS_READY, args->meta);
+#endif // BRPC_BTHREAD_TRACER
     return tls_task_group->control()->push_priority_queue(args->tag, args->meta->tid);
 }
 
@@ -1031,14 +1035,15 @@ int TaskGroup::interrupt(bthread_t tid, TaskControl* c, bthread_tag_t tag) {
         }
     } else if (sleep_id != 0) {
         if (get_global_timer_thread()->unschedule(sleep_id) == 0) {
-            bthread::TaskGroup* g = bthread::tls_task_group;
+            TaskGroup* g = tls_task_group;
+            TaskMeta* m = address_meta(tid);
             if (g) {
-                g->ready_to_run(TaskGroup::address_meta(tid));
+                g->ready_to_run(m);
             } else {
                 if (!c) {
                     return EINVAL;
                 }
-                c->choose_one_group(tag)->ready_to_run_remote(TaskGroup::address_meta(tid));
+                c->choose_one_group(tag)->ready_to_run_remote(m);
             }
         }
     }