diff --git a/be/src/util/once.h b/be/src/util/once.h index 5031daac69eca3..c9ac9df1b00c5b 100644 --- a/be/src/util/once.h +++ b/be/src/util/once.h @@ -22,6 +22,7 @@ #include #include +#include #include "common/exception.h" #include "olap/olap_common.h" @@ -52,17 +53,57 @@ class DorisCallOnce { public: DorisCallOnce() : _has_called(false) {} + // this method is not exception safe, it will core when exception occurs in + // callback method. I have tested the code https://en.cppreference.com/w/cpp/thread/call_once. // If the underlying `once_flag` has yet to be invoked, invokes the provided // lambda and stores its return value. Otherwise, returns the stored Status. + // template + // ReturnType call(Fn fn) { + // std::call_once(_once_flag, [this, fn] { + // _status = fn(); + // _has_called.store(true, std::memory_order_release); + // }); + // return _status; + // } + + // If exception occurs in the function, the call flag is set, if user call + // it again, the same exception will be thrown. + // It is different from std::call_once. This is because if a method is called once + // some internal state is changed, it maybe not called again although exception + // occurred. template ReturnType call(Fn fn) { - std::call_once(_once_flag, [this, fn] { + // Avoid lock to improve performance + if (has_called()) { + if (_eptr) { + std::rethrow_exception(_eptr); + } + return _status; + } + std::lock_guard l(_flag_lock); + // should check again because maybe another thread call successfully. + if (has_called()) { + if (_eptr) { + std::rethrow_exception(_eptr); + } + return _status; + } + try { _status = fn(); + } catch (...) { + // Save the exception for next call. + _eptr = std::current_exception(); _has_called.store(true, std::memory_order_release); - }); + std::rethrow_exception(_eptr); + } + // This memory order make sure both status and eptr is set + // and will be seen in another thread. + _has_called.store(true, std::memory_order_release); return _status; } + // Has to pay attention to memory order + // see https://en.cppreference.com/w/cpp/atomic/memory_order // Return whether `call` has been invoked or not. bool has_called() const { // std::memory_order_acquire here and std::memory_order_release in @@ -72,11 +113,22 @@ class DorisCallOnce { } // Return the stored result. The result is only meaningful when `has_called() == true`. - ReturnType stored_result() const { return _status; } + ReturnType stored_result() const { + if (!has_called()) { + // Could not return status if the method not called. + throw std::exception(); + } + if (_eptr) { + std::rethrow_exception(_eptr); + } + return _status; + } private: std::atomic _has_called; - std::once_flag _once_flag; + // std::once_flag _once_flag; + std::mutex _flag_lock; + std::exception_ptr _eptr; ReturnType _status; }; diff --git a/be/test/util/doris_callonce_test.cpp b/be/test/util/doris_callonce_test.cpp new file mode 100644 index 00000000000000..e9644f161752b5 --- /dev/null +++ b/be/test/util/doris_callonce_test.cpp @@ -0,0 +1,152 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you 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 License for the +// specific language governing permissions and limitations +// under the License. + +#include +#include +#include + +#include +#include +#include +#include +#include + +#include "common/status.h" +#include "gtest/gtest_pred_impl.h" +#include "util/once.h" + +namespace doris { +class DorisCallOnceTest : public ::testing::Test {}; + +TEST_F(DorisCallOnceTest, TestNormal) { + DorisCallOnce call1; + EXPECT_EQ(call1.has_called(), false); + + Status st = call1.call([&]() -> Status { return Status::OK(); }); + EXPECT_EQ(call1.has_called(), true); + EXPECT_EQ(call1.stored_result().code(), ErrorCode::OK); + + st = call1.call([&]() -> Status { return Status::InternalError(""); }); + EXPECT_EQ(call1.has_called(), true); + // The error code should not changed + EXPECT_EQ(call1.stored_result().code(), ErrorCode::OK); +} + +// Test that, if the string contents is shorter than the initial capacity +// of the faststring, shrink_to_fit() leaves the string in the built-in +// array. +TEST_F(DorisCallOnceTest, TestErrorHappens) { + DorisCallOnce call1; + EXPECT_EQ(call1.has_called(), false); + + Status st = call1.call([&]() -> Status { return Status::InternalError(""); }); + EXPECT_EQ(call1.has_called(), true); + EXPECT_EQ(call1.stored_result().code(), ErrorCode::INTERNAL_ERROR); + + st = call1.call([&]() -> Status { return Status::OK(); }); + EXPECT_EQ(call1.has_called(), true); + // The error code should not changed + EXPECT_EQ(call1.stored_result().code(), ErrorCode::INTERNAL_ERROR); +} + +TEST_F(DorisCallOnceTest, TestExceptionHappens) { + DorisCallOnce call1; + EXPECT_EQ(call1.has_called(), false); + bool exception_occured = false; + bool runtime_occured = false; + try { + Status st = call1.call([&]() -> Status { + throw std::exception(); + return Status::InternalError(""); + }); + } catch (...) { + // Exception has to throw to the call method + exception_occured = true; + } + EXPECT_EQ(exception_occured, true); + EXPECT_EQ(call1.has_called(), true); + + // call again, should throw the same exception. + exception_occured = false; + runtime_occured = false; + try { + Status st = call1.call([&]() -> Status { return Status::InternalError(""); }); + } catch (...) { + // Exception has to throw to the call method + exception_occured = true; + } + EXPECT_EQ(exception_occured, true); + EXPECT_EQ(call1.has_called(), true); + + // If call get result, should catch exception. + exception_occured = false; + runtime_occured = false; + try { + Status st = call1.stored_result(); + } catch (...) { + // Exception has to throw to the call method + exception_occured = true; + } + EXPECT_EQ(exception_occured, true); + + // Test the exception should actually the same one throwed by the callback method. + DorisCallOnce call2; + exception_occured = false; + runtime_occured = false; + try { + try { + Status st = call2.call([&]() -> Status { + throw std::runtime_error("runtime error happens"); + return Status::InternalError(""); + }); + } catch (const std::runtime_error&) { + // Exception has to throw to the call method + runtime_occured = true; + } + } catch (...) { + // Exception has to throw to the call method, the runtime error is captured, + // so this code will not hit. + exception_occured = true; + } + EXPECT_EQ(runtime_occured, true); + EXPECT_EQ(exception_occured, false); + + // Test the exception should actually the same one throwed by the callback method. + DorisCallOnce call3; + exception_occured = false; + runtime_occured = false; + try { + try { + Status st = call3.call([&]() -> Status { + throw std::exception(); + return Status::InternalError(""); + }); + } catch (const std::runtime_error&) { + // Exception has to throw to the call method, but not runtime error + // so that this code will not hit + runtime_occured = true; + } + } catch (...) { + // Exception has to throw to the call method, the runtime error is not captured, + // so this code will hit. + exception_occured = true; + } + EXPECT_EQ(runtime_occured, false); + EXPECT_EQ(exception_occured, true); +} + +} // namespace doris