From a397c07edb7144958e33266d44133b336b75d499 Mon Sep 17 00:00:00 2001
From: Dan Wang <wangdan@apache.org>
Date: Mon, 25 Dec 2023 14:31:26 +0800
Subject: [PATCH] fix: standby meta server exits abnormally with core dump
 after receiving the http request `/meta/cluster` (#1816)

https://github.com/apache/incubator-pegasus/issues/1817

If meta server is built without testing(i.e. `./run.sh build` without `--test`),
the macro `MOCK_TEST` dedicated to test code should not be enabled,
which might lead to unexpected behaviors.

On the other hand, meta server should not exit abnormally with core
dump even if it is built for testing(i.e. `./run.sh build --test`). Once it
finds that balancer is not initialized by mocking, it should go through
the normal process to prevent from core dump.
---
 cmake_modules/BaseFunctions.cmake |  4 ++++
 src/meta/CMakeLists.txt           |  2 --
 src/meta/meta_http_service.cpp    | 26 ++++++++++++++++++++++----
 src/utils/api_utilities.h         |  2 +-
 src/utils/metrics.h               |  2 +-
 5 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/cmake_modules/BaseFunctions.cmake b/cmake_modules/BaseFunctions.cmake
index dc0e3b28f7..b1dff9b601 100644
--- a/cmake_modules/BaseFunctions.cmake
+++ b/cmake_modules/BaseFunctions.cmake
@@ -205,6 +205,10 @@ function(dsn_setup_compiler_flags)
   # We want access to the PRI* print format macros.
   add_definitions(-D__STDC_FORMAT_MACROS)
 
+  if(${BUILD_TEST})
+    add_definitions(-DMOCK_TEST)
+  endif()
+
   set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++17 -gdwarf-4" CACHE STRING "" FORCE)
 
   #  -Wall: Enable all warnings.
diff --git a/src/meta/CMakeLists.txt b/src/meta/CMakeLists.txt
index fcc3a12329..a52a607f66 100644
--- a/src/meta/CMakeLists.txt
+++ b/src/meta/CMakeLists.txt
@@ -57,8 +57,6 @@ set(MY_BOOST_LIBS Boost::system Boost::filesystem)
 # Extra files that will be installed
 set(MY_BINPLACES "")
 
-add_definitions(-DDSN_MOCK_TEST)
-
 dsn_add_shared_library()
 
 add_subdirectory(test)
diff --git a/src/meta/meta_http_service.cpp b/src/meta/meta_http_service.cpp
index 902822d79f..400fd160ed 100644
--- a/src/meta/meta_http_service.cpp
+++ b/src/meta/meta_http_service.cpp
@@ -460,8 +460,9 @@ void meta_http_service::list_node_handler(const http_request &req, http_response
 
 void meta_http_service::get_cluster_info_handler(const http_request &req, http_response &resp)
 {
-    if (!redirect_if_not_primary(req, resp))
+    if (!redirect_if_not_primary(req, resp)) {
         return;
+    }
 
     dsn::utils::table_printer tp;
     std::ostringstream out;
@@ -827,12 +828,29 @@ void meta_http_service::update_scenario_handler(const http_request &req, http_re
 
 bool meta_http_service::redirect_if_not_primary(const http_request &req, http_response &resp)
 {
-#ifdef DSN_MOCK_TEST
-    return true;
+#ifdef MOCK_TEST
+    // To enable MOCK_TEST, the option BUILD_TEST for cmake should be opened by:
+    //     cmake -DBUILD_TEST=ON ...
+    // which could be done by building Pegasus with tests by:
+    //     ./run.sh build --test ...
+    //
+    // If `_service->_balancer` is not null, it must has been initialized by mocking, in which case
+    // just returning true is ok.
+    //
+    // Otherwise, once `_service->_balancer` is null, which means this must be a standby meta
+    // server, returning true would lead to coredump due to null `_service->_balancer` while
+    // processing requests in `get_cluster_info_handler`. Thus it should go through the following
+    // normal process instead of just returning true.
+    if (_service->_balancer) {
+        return true;
+    }
 #endif
+
     rpc_address leader;
-    if (_service->_failure_detector->get_leader(&leader))
+    if (_service->_failure_detector->get_leader(&leader)) {
         return true;
+    }
+
     // set redirect response
     resp.location = "http://" + leader.to_std_string() + '/' + req.path;
     if (!req.query_args.empty()) {
diff --git a/src/utils/api_utilities.h b/src/utils/api_utilities.h
index 29cf232a47..537ceaaedd 100644
--- a/src/utils/api_utilities.h
+++ b/src/utils/api_utilities.h
@@ -84,7 +84,7 @@ extern void dsn_coredump();
         }                                                                                          \
     } while (0)
 
-#ifdef DSN_MOCK_TEST
+#ifdef MOCK_TEST
 #define mock_private public
 #define mock_virtual virtual
 #else
diff --git a/src/utils/metrics.h b/src/utils/metrics.h
index d2789e51b3..93ed703e7d 100644
--- a/src/utils/metrics.h
+++ b/src/utils/metrics.h
@@ -1404,7 +1404,7 @@ class percentile : public closeable_metric
             _full_nth_elements[i].store(value_type{}, std::memory_order_relaxed);
         }
 
-#ifdef DSN_MOCK_TEST
+#ifdef MOCK_TEST
         if (interval_ms == 0) {
             // Timer is disabled.
             return;