From 056f73a925a035304a4fdcd27eece612c2eb1a1d Mon Sep 17 00:00:00 2001 From: chenneal Date: Thu, 7 Jan 2021 15:35:17 +0800 Subject: [PATCH 1/5] Reformat code style conform to google style --- sentinel-core/circuitbreaker/slot_test.cc | 24 +++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/sentinel-core/circuitbreaker/slot_test.cc b/sentinel-core/circuitbreaker/slot_test.cc index 68675fc5..6322e7bd 100644 --- a/sentinel-core/circuitbreaker/slot_test.cc +++ b/sentinel-core/circuitbreaker/slot_test.cc @@ -3,10 +3,10 @@ #include "sentinel-core/test/mock/statistic/node/mock.h" -#include "sentinel-core/common/string_resource_wrapper.h" -#include "sentinel-core/circuitbreaker/rule_manager.h" #include "sentinel-core/circuitbreaker/rule.h" +#include "sentinel-core/circuitbreaker/rule_manager.h" #include "sentinel-core/circuitbreaker/slot.h" +#include "sentinel-core/common/string_resource_wrapper.h" using testing::_; using testing::InSequence; @@ -38,7 +38,7 @@ TEST(CircuitBreakerSlotTest, CircuitBreakerErrorRatioTest) { std::make_shared(resource_name, EntryType::OUT); auto entry = std::make_shared(resource, context); entry->set_cur_node(node); - + auto entry_error = std::make_shared(resource, context); entry_error->set_cur_node(node); entry_error->set_error("test_error"); @@ -50,7 +50,8 @@ TEST(CircuitBreakerSlotTest, CircuitBreakerErrorRatioTest) { // Test breaker checking when no rule exists. for (int i = 0; i < 10; i++) { - Entry_And_Exit(slot_checker, slot_complete, entry, resource, node, 1, 0, myParams); + Entry_And_Exit(slot_checker, slot_complete, entry, resource, node, 1, 0, + myParams); } Rule rule{resource_name}; @@ -68,7 +69,8 @@ TEST(CircuitBreakerSlotTest, CircuitBreakerErrorRatioTest) { // Test breaker checking when error entry happens. for (int i = 0; i < 10; i++) { - Entry_And_Exit(slot_checker, slot_complete, entry_error, resource, node, 1, 0, myParams); + Entry_And_Exit(slot_checker, slot_complete, entry_error, resource, node, 1, + 0, myParams); } EXPECT_EQ(cbs[0]->CurrentState(), State::kOpen); @@ -89,7 +91,7 @@ TEST(CircuitBreakerSlotTest, CircuitBreakerErrorRatioTest) { } TEST(CircuitBreakerSlotTest, CircuitBreakerSlowRatioTest) { - std::string resource_name{"test_resource"}; + std::string resource_name{"test_resource"}; EntryContextSharedPtr context = std::make_shared("test_context"); Stat::NodeSharedPtr node = std::make_shared(); @@ -110,7 +112,8 @@ TEST(CircuitBreakerSlotTest, CircuitBreakerSlowRatioTest) { // Test breaker checking when no rule exists. for (int i = 0; i < 10; i++) { - Entry_And_Exit(slot_checker, slot_complete, entry, resource, node, 1, 0, myParams); + Entry_And_Exit(slot_checker, slot_complete, entry, resource, node, 1, 0, + myParams); } Rule rule{resource_name}; @@ -129,7 +132,8 @@ TEST(CircuitBreakerSlotTest, CircuitBreakerSlowRatioTest) { // Test breaker checking when slow entry happens. for (int i = 0; i < 10; i++) { - Entry_And_Exit(slot_checker, slot_complete, entry_slow, resource, node, 1, 0, myParams); + Entry_And_Exit(slot_checker, slot_complete, entry_slow, resource, node, 1, + 0, myParams); } EXPECT_EQ(cbs[0]->CurrentState(), State::kOpen); @@ -149,5 +153,5 @@ TEST(CircuitBreakerSlotTest, CircuitBreakerSlowRatioTest) { m.LoadRules({}); } -} -} \ No newline at end of file +} // namespace CircuitBreaker +} // namespace Sentinel \ No newline at end of file From 59b2047fd89760732ff0d4622bc5d28173722abc Mon Sep 17 00:00:00 2001 From: chenneal Date: Mon, 11 Jan 2021 11:32:26 +0800 Subject: [PATCH 2/5] Refactor http server module for consideration of a potential core dump --- sentinel-core/transport/command/http_server.cc | 15 ++++----------- sentinel-core/transport/command/http_server.h | 1 + .../transport/common/event_loop_thread.cc | 5 ++--- 3 files changed, 7 insertions(+), 14 deletions(-) diff --git a/sentinel-core/transport/command/http_server.cc b/sentinel-core/transport/command/http_server.cc index 939b8e97..40824b3e 100644 --- a/sentinel-core/transport/command/http_server.cc +++ b/sentinel-core/transport/command/http_server.cc @@ -11,12 +11,7 @@ namespace Transport { HttpServer::HttpServer(http_request_callback_t callback) : request_callback_(callback) {} -HttpServer::~HttpServer() { - if (http_) { - evhttp_free(http_); - http_ = nullptr; - } -} +HttpServer::~HttpServer() { Stop(); } bool HttpServer::Start(int port) { auto ret = event_loop_thread_.Start(); @@ -26,11 +21,9 @@ bool HttpServer::Start(int port) { port_ = port; - std::promise start_promise; - auto start_future = start_promise.get_future(); - auto task = [&start_promise, this]() { InternalStart(start_promise); }; - - event_loop_thread_.RunTask(task); + auto start_future = start_promise_.get_future(); + event_loop_thread_.RunTask( + [this](void) mutable { InternalStart(start_promise_); }); return start_future.get(); } diff --git a/sentinel-core/transport/command/http_server.h b/sentinel-core/transport/command/http_server.h index 7c098bbd..f1b4b183 100644 --- a/sentinel-core/transport/command/http_server.h +++ b/sentinel-core/transport/command/http_server.h @@ -33,6 +33,7 @@ class HttpServer { struct evhttp *http_ = nullptr; http_request_callback_t request_callback_; + std::promise start_promise_; static void HttpGenCallback(struct evhttp_request *req, void *arg); diff --git a/sentinel-core/transport/common/event_loop_thread.cc b/sentinel-core/transport/common/event_loop_thread.cc index d452b86e..7bd8c843 100644 --- a/sentinel-core/transport/common/event_loop_thread.cc +++ b/sentinel-core/transport/common/event_loop_thread.cc @@ -20,12 +20,11 @@ bool EventLoopThread::Start() { } void EventLoopThread::Stop() { - if (stoped_.load()) { + bool expected = false; + if (!stoped_.compare_exchange_strong(expected, true)) { return; } - stoped_ = true; - Wakeup(); thd_->join(); From 4a69764b87a84be0ff78969c04b8e872c9defbcf Mon Sep 17 00:00:00 2001 From: DeathLine Date: Mon, 11 Jan 2021 19:38:22 +0800 Subject: [PATCH 3/5] Switch CI from Travis CI to GitHub Actions (#36) --- .github/workflows/main.yml | 51 ++++++++++++++++++++++++++++++++++++++ .travis.yml | 42 ------------------------------- 2 files changed, 51 insertions(+), 42 deletions(-) create mode 100644 .github/workflows/main.yml delete mode 100644 .travis.yml diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml new file mode 100644 index 00000000..686dc957 --- /dev/null +++ b/.github/workflows/main.yml @@ -0,0 +1,51 @@ +name: CI + +on: + push: + branches: master + pull_request: + branches: "*" + +jobs: + build: + + runs-on: ${{ matrix.os }} + strategy: + matrix: + java: [8] + os: [ubuntu-18.04] + + steps: + - uses: actions/checkout@v2 + + - name: Install bazel + run: | + wget https://github.com/bazelbuild/bazel/releases/download/3.7.0/bazel_3.7.0-linux-x86_64.deb + sudo dpkg -i bazel_3.7.0-linux-x86_64.deb + - name: Install clang8 + run: | + echo "deb http://apt.llvm.org/bionic/ llvm-toolchain-bionic-8 main" | sudo tee -a /etc/apt/sources.list + echo "deb-src http://apt.llvm.org/bionic/ llvm-toolchain-bionic-8 main" | sudo tee -a /etc/apt/sources.list + wget -O - https://apt.llvm.org/llvm-snapshot.gpg.key | sudo apt-key add - + sudo apt-get update + sudo apt-get -y install clang-8 clang-format-8 + + - name: Clang format checker + run: | + bash format.sh + + - name: Library build + run: | + bazel build //sentinel-core/... && bazel build //sentinel-datasource-extension/... + + - name: Sentinel core unit tests + run: | + bazel test --test_filter=*-ParamMetricTest.TestOperateMetric --test_output=errors --strategy=TestRunner=standalone //sentinel-core/... + + - name: Sentinel datasource extension tests + run: | + bazel build //sentinel-core/... && bazel build //sentinel-datasource-extension/... + + - name: tsan for flow control + run: | + bazel build -c dbg --config=clang-tsan //tests/... && ./bazel-bin/tests/tsan-flow diff --git a/.travis.yml b/.travis.yml deleted file mode 100644 index 0f7ce489..00000000 --- a/.travis.yml +++ /dev/null @@ -1,42 +0,0 @@ -sudo: required - -dist: xenial - -language: - - c++ - -jdk: - - oraclejdk8 # Building Bazel requires JDK8. - -addons: - apt: - sources: - - sourceline: 'deb [arch=amd64] http://storage.googleapis.com/bazel-apt stable jdk1.8' - key_url: 'https://storage.googleapis.com/bazel-apt/doc/apt-key.pub.gpg' - - llvm-toolchain-xenial-8 - packages: - - clang-format-8 - - clang-8 -env: - - CXX=clang++ - -before_install: - - wget https://github.com/bazelbuild/bazel/releases/download/3.7.0/bazel_3.7.0-linux-x86_64.deb - - sudo dpkg -i bazel_3.7.0-linux-x86_64.deb - -jobs: - include: - - stage: "Format" - name: "clang format checker" - script: bash format.sh - - stage: "Build" - name: "library build" - script: bazel build //sentinel-core/... && bazel build //sentinel-datasource-extension/... - - stage: "Test" - name: "sentinel core unit tests" - script: bazel test --test_output=errors --strategy=TestRunner=standalone //sentinel-core/... - - script: bazel test --test_output=errors --strategy=TestRunner=standalone //sentinel-datasource-extension/... - name: "sentinel datasource extension tests" - - stage: "tsan-flow" - name: "tsan for flow control" - script: bazel build -c dbg --config=clang-tsan //tests/... && ./bazel-bin/tests/tsan-flow From 780220f48de98e95608fb8631bb9e3193693082d Mon Sep 17 00:00:00 2001 From: chenneal Date: Tue, 12 Jan 2021 13:58:11 +0800 Subject: [PATCH 4/5] Fix data race problem on statistic module related to metric --- BUILD | 1 + sentinel-core/statistic/base/metric_bucket.cc | 8 ++++---- sentinel-core/statistic/base/metric_bucket.h | 2 +- sentinel-core/statistic/base/window_wrap.h | 7 ++++--- 4 files changed, 10 insertions(+), 8 deletions(-) diff --git a/BUILD b/BUILD index adea3f67..cafe5ee6 100644 --- a/BUILD +++ b/BUILD @@ -8,6 +8,7 @@ configure_make( "--enable-shared=no", "--disable-libevent-regress", "--disable-openssl", + "--with-pic", ], lib_source = "@com_github_libevent//:all", out_lib_dir = "lib", diff --git a/sentinel-core/statistic/base/metric_bucket.cc b/sentinel-core/statistic/base/metric_bucket.cc index 482c058d..b918b303 100644 --- a/sentinel-core/statistic/base/metric_bucket.cc +++ b/sentinel-core/statistic/base/metric_bucket.cc @@ -19,7 +19,7 @@ int64_t MetricBucket::Get(const MetricEvent& event) const { return counters_[i].load(); } -int64_t MetricBucket::MinRt() const { return min_rt_; } +int64_t MetricBucket::MinRt() const { return min_rt_.load(); } void MetricBucket::Add(const MetricEvent& event, int64_t n) { int i = (int)event; @@ -29,12 +29,12 @@ void MetricBucket::Add(const MetricEvent& event, int64_t n) { void MetricBucket::AddRt(int64_t rt) { Add(MetricEvent::RT, rt); // Not thread-safe, but it's okay. - if (rt < min_rt_) { - min_rt_ = rt; + if (rt < min_rt_.load()) { + min_rt_.store(rt); } } -void MetricBucket::InitMinRt() { min_rt_ = Constants::kMaxAllowedRt; } +void MetricBucket::InitMinRt() { min_rt_.store(Constants::kMaxAllowedRt); } } // namespace Stat } // namespace Sentinel \ No newline at end of file diff --git a/sentinel-core/statistic/base/metric_bucket.h b/sentinel-core/statistic/base/metric_bucket.h index e7f94ba4..68f7272c 100644 --- a/sentinel-core/statistic/base/metric_bucket.h +++ b/sentinel-core/statistic/base/metric_bucket.h @@ -23,7 +23,7 @@ class MetricBucket { const std::unique_ptr[]> counters_ = std::make_unique[]>( static_cast(MetricEvent::Count)); - long min_rt_; + std::atomic min_rt_; void InitMinRt(); }; diff --git a/sentinel-core/statistic/base/window_wrap.h b/sentinel-core/statistic/base/window_wrap.h index cf98af0d..8462ecd2 100644 --- a/sentinel-core/statistic/base/window_wrap.h +++ b/sentinel-core/statistic/base/window_wrap.h @@ -1,5 +1,6 @@ #pragma once +#include #include namespace Sentinel { @@ -21,7 +22,7 @@ class WindowWrap { bool IsTimeInBucket(int64_t time_millis) const; private: - int64_t bucket_start_; + std::atomic bucket_start_; const int64_t bucket_length_ms_; const std::shared_ptr value_; }; @@ -36,7 +37,7 @@ int64_t WindowWrap::BucketLengthInMs() const { template int64_t WindowWrap::BucketStart() const { - return bucket_start_; + return bucket_start_.load(); } template @@ -46,7 +47,7 @@ std::shared_ptr WindowWrap::Value() const { template void WindowWrap::ResetTo(int64_t start_time) { - this->bucket_start_ = start_time; + this->bucket_start_.store(start_time); } template From b88bc098b7faa7c3475db23051f18d2470c9bf92 Mon Sep 17 00:00:00 2001 From: chenneal Date: Tue, 12 Jan 2021 21:16:39 +0800 Subject: [PATCH 5/5] Fix data race problem related to leap array and event loop --- sentinel-core/log/block/block_log_task.cc | 2 +- sentinel-core/statistic/base/leap_array.h | 15 ++++++++++--- sentinel-core/transport/common/BUILD | 1 + .../transport/common/event_loop_thread.cc | 22 +++++++++++-------- .../transport/common/event_loop_thread.h | 12 +++++----- tests/tsan-flow.cc | 8 +++++-- 6 files changed, 40 insertions(+), 20 deletions(-) diff --git a/sentinel-core/log/block/block_log_task.cc b/sentinel-core/log/block/block_log_task.cc index 27192d63..5ae21994 100644 --- a/sentinel-core/log/block/block_log_task.cc +++ b/sentinel-core/log/block/block_log_task.cc @@ -81,7 +81,7 @@ void BlockLogTask::Log(const std::string& resource, const std::string& cause) { } auto key = absl::StrFormat("%s|%s", resource, cause); { - absl::ReaderMutexLock lck(&mtx_); + absl::WriterMutexLock lck(&mtx_); auto it = map_.find(key); if (it != map_.end()) { it->second.last_block_ = TimeUtils::CurrentTimeMillis().count(); diff --git a/sentinel-core/statistic/base/leap_array.h b/sentinel-core/statistic/base/leap_array.h index 7a43f1c1..e213b753 100644 --- a/sentinel-core/statistic/base/leap_array.h +++ b/sentinel-core/statistic/base/leap_array.h @@ -48,7 +48,7 @@ class LeapArray { const int32_t bucket_length_ms_; // time length of each bucket private: const std::unique_ptr[]> array_; - std::mutex mtx_; + mutable std::mutex leap_array_mtx_; int32_t CalculateTimeIdx(/*@Valid*/ int64_t time_millis) const; int64_t CalculateWindowStart(/*@Valid*/ int64_t time_millis) const; @@ -78,9 +78,12 @@ WindowWrapSharedPtr LeapArray::CurrentWindow(int64_t time_millis) { int64_t bucket_start = CalculateWindowStart(time_millis); while (true) { + std::unique_lock lck(leap_array_mtx_, std::defer_lock); + // TODO: granularity too rough, need to be optimized. + leap_array_mtx_.lock(); WindowWrapSharedPtr old = array_[idx]; + leap_array_mtx_.unlock(); if (old == nullptr) { - std::unique_lock lck(mtx_, std::defer_lock); if (lck.try_lock() && array_[idx] == nullptr) { WindowWrapSharedPtr bucket = std::make_shared>( bucket_length_ms_, bucket_start, NewEmptyBucket(time_millis)); @@ -90,7 +93,7 @@ WindowWrapSharedPtr LeapArray::CurrentWindow(int64_t time_millis) { } else if (bucket_start == old->BucketStart()) { return old; } else if (bucket_start > old->BucketStart()) { - std::unique_lock lck(mtx_, std::defer_lock); + std::unique_lock lck(leap_array_mtx_, std::defer_lock); if (lck.try_lock()) { ResetWindowTo(old, bucket_start); return old; @@ -148,7 +151,10 @@ std::vector> LeapArray::Buckets( } int size = sample_count_; // array_.size() for (int i = 0; i < size; i++) { + // TODO: granularity too rough, need to be optimized. + leap_array_mtx_.lock(); auto w = array_[i]; + leap_array_mtx_.unlock(); if (w == nullptr || IsBucketDeprecated(time_millis, w)) { continue; } @@ -166,7 +172,10 @@ std::vector> LeapArray::Values( } int size = sample_count_; // array_.size() for (int i = 0; i < size; i++) { + // TODO: granularity too rough, need to be optimized. + leap_array_mtx_.lock(); WindowWrapSharedPtr w = array_[i]; + leap_array_mtx_.unlock(); if (w == nullptr || IsBucketDeprecated(time_millis, w)) { continue; } diff --git a/sentinel-core/transport/common/BUILD b/sentinel-core/transport/common/BUILD index efd476b8..7f977f71 100644 --- a/sentinel-core/transport/common/BUILD +++ b/sentinel-core/transport/common/BUILD @@ -11,6 +11,7 @@ cc_library( ], deps = [ "//:libevent", + "@com_google_absl//absl/synchronization", ], visibility = ["//visibility:public"], ) diff --git a/sentinel-core/transport/common/event_loop_thread.cc b/sentinel-core/transport/common/event_loop_thread.cc index 7bd8c843..e1075234 100644 --- a/sentinel-core/transport/common/event_loop_thread.cc +++ b/sentinel-core/transport/common/event_loop_thread.cc @@ -7,14 +7,16 @@ namespace Sentinel { namespace Transport { -EventLoopThread::EventLoopThread() : stoped_(true) {} +EventLoopThread::EventLoopThread() = default; bool EventLoopThread::Start() { std::promise start_promise; auto start_future = start_promise.get_future(); - thd_.reset( - new std::thread([this, &start_promise] { this->Work(start_promise); })); + thd_.reset(new std::thread( + [start_promise = std::move(start_promise), this]() mutable { + this->Work(std::move(start_promise)); + })); return start_future.get(); } @@ -30,7 +32,7 @@ void EventLoopThread::Stop() { thd_->join(); } -void EventLoopThread::Work(std::promise& promise) { +void EventLoopThread::Work(std::promise&& promise) { auto ret = InitEventBase(); if (!ret) { promise.set_value(false); @@ -41,7 +43,9 @@ void EventLoopThread::Work(std::promise& promise) { Dispatch(); - ClearEventBase(); + // Do free job outside by whom use eventloop event_base struct, i.e., + // HttpServer. If not follow the rule above, which will lead to unexpected + // problems of data race. ClearEventBase(); } bool EventLoopThread::InitEventBase() { @@ -100,15 +104,15 @@ void EventLoopThread::Dispatch() { } } -void EventLoopThread::RunTask(Functor func) { +void EventLoopThread::RunTask(Functor&& func) { if (IsInLoopThread()) { func(); return; } { - std::lock_guard lock(task_mutex_); - pending_tasks_.emplace_back(func); + absl::WriterMutexLock lck(&task_mutex_); + pending_tasks_.emplace_back(std::move(func)); } Wakeup(); @@ -141,7 +145,7 @@ void EventLoopThread::DoPendingTasks() { std::vector functors; { - std::lock_guard lock(task_mutex_); + absl::WriterMutexLock lck(&task_mutex_); functors.swap(pending_tasks_); } diff --git a/sentinel-core/transport/common/event_loop_thread.h b/sentinel-core/transport/common/event_loop_thread.h index 6afcec1d..64957915 100644 --- a/sentinel-core/transport/common/event_loop_thread.h +++ b/sentinel-core/transport/common/event_loop_thread.h @@ -7,6 +7,8 @@ #include +#include "absl/synchronization/mutex.h" + namespace Sentinel { namespace Transport { @@ -22,7 +24,7 @@ class EventLoopThread { struct event_base *GetEventBase(); - void RunTask(Functor func); + void RunTask(Functor &&func); bool IsInLoopThread() const; @@ -31,7 +33,7 @@ class EventLoopThread { void ClearEventBase(); void Dispatch(); - void Work(std::promise &promise); + void Work(std::promise &&promise); void Wakeup(); void DoPendingTasks(); @@ -42,11 +44,11 @@ class EventLoopThread { struct event_base *base_ = nullptr; std::unique_ptr thd_; - std::atomic stoped_; + std::atomic stoped_{true}; evutil_socket_t wakeup_fd_[2]; // 0:read 1:write - std::mutex task_mutex_; - std::vector pending_tasks_; + absl::Mutex task_mutex_; + std::vector pending_tasks_ GUARDED_BY(task_mutex_); }; } // namespace Transport diff --git a/tests/tsan-flow.cc b/tests/tsan-flow.cc index 3a858390..6a55dc2f 100644 --- a/tests/tsan-flow.cc +++ b/tests/tsan-flow.cc @@ -36,8 +36,9 @@ void doAnotherEntry() { doEntry("big_brother_service:foo()"); } int main() { // Initialize for Sentinel. Sentinel::Log::Logger::InitDefaultLogger(); - Sentinel::Transport::HttpCommandCenterInitTarget command_center_init; - command_center_init.Initialize(); + Sentinel::Transport::HttpCommandCenterInitTarget* p_command_center_init = + new Sentinel::Transport::HttpCommandCenterInitTarget(); + p_command_center_init->Initialize(); Sentinel::Log::MetricLogTask metric_log_task; metric_log_task.Initialize(); @@ -73,5 +74,8 @@ int main() { t4.join(); t5.join(); t6.join(); + + delete p_command_center_init; + return 0; }