Repository navigation
Fix URMA polling shutdown lifetime #3634
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,26 +21,180 @@ | |
| #include <gflags/gflags.h> | ||
|
|
||
| #if BRPC_WITH_URMA | ||
| #include <atomic> | ||
| #include <cstdlib> | ||
| #include "butil/atomicops.h" | ||
| #include "butil/sys_byteorder.h" | ||
| #include "urma_api.h" | ||
| #include "brpc/urma/urma_endpoint.h" | ||
| #include "brpc/urma/urma_handshake.h" | ||
| #include "brpc/urma/urma_handshake.pb.h" | ||
| #include "brpc/urma/urma_helper.h" | ||
| #include "urma_types.h" | ||
|
|
||
| using namespace brpc; | ||
|
|
||
| DECLARE_int32(task_group_ntags); | ||
|
|
||
| namespace brpc { | ||
| namespace urma { | ||
|
|
||
| DECLARE_int32(urma_client_handshake_version); | ||
| DECLARE_int32(urma_buffer_size); | ||
| DECLARE_int32(urma_buffer_count); | ||
| DECLARE_int32(urma_sq_size); | ||
| DECLARE_int32(urma_rq_size); | ||
| DECLARE_int32(urma_prepared_jetty_cnt); | ||
| extern bool g_skip_urma_init; | ||
| extern butil::atomic<bool> g_urma_available; | ||
|
|
||
| } // namespace urma | ||
| } // namespace brpc | ||
|
|
||
| class UrmaPollingTest : public ::testing::TestWithParam<bool> { | ||
| protected: | ||
| void SetUp() override { | ||
| urma::FLAGS_urma_use_polling = true; | ||
| urma::FLAGS_urma_poller_num = 2; | ||
| urma::FLAGS_urma_disable_bthread = GetParam(); | ||
| ASSERT_EQ(0, urma::UrmaEndpoint::GlobalInitialize()); | ||
| } | ||
|
|
||
| void TearDown() override { | ||
| urma::UrmaEndpoint::PollingModeRelease(0); | ||
| } | ||
|
|
||
| GFLAGS_NAMESPACE::FlagSaver _flags; | ||
| }; | ||
|
|
||
| TEST_P(UrmaPollingTest, release_joins_and_can_be_repeated) { | ||
| std::atomic<int> initialized{0}; | ||
| std::atomic<int> released{0}; | ||
| ASSERT_EQ(0, urma::UrmaEndpoint::PollingModeInitialize( | ||
| 0, nullptr, [&] { ++initialized; }, [&] { ++released; })); | ||
| urma::UrmaEndpoint::PollingModeRelease(0); | ||
| EXPECT_EQ(2, initialized.load()); | ||
| EXPECT_EQ(2, released.load()); | ||
| auto& group = urma::UrmaEndpoint::PollerGroups()[0]; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [replied by brpc-oncall robot] The new tests access members that are private in |
||
| EXPECT_FALSE(group.running.load()); | ||
| for (const auto& poller : group.pollers) { | ||
| EXPECT_EQ(INVALID_BTHREAD, poller.tid); | ||
| } | ||
|
Comment on lines
+78
to
+82
|
||
| urma::UrmaEndpoint::PollingModeRelease(0); | ||
| EXPECT_EQ(2, released.load()); | ||
|
|
||
| ASSERT_EQ(0, urma::UrmaEndpoint::PollingModeInitialize( | ||
| 0, nullptr, nullptr, [&] { ++released; })); | ||
| urma::UrmaEndpoint::PollingModeRelease(0); | ||
| EXPECT_EQ(4, released.load()); | ||
| } | ||
|
|
||
| TEST_P(UrmaPollingTest, late_cq_operations_skip_stopped_pollers) { | ||
| ASSERT_EQ(0, urma::UrmaEndpoint::PollingModeInitialize( | ||
| 0, nullptr, nullptr, nullptr)); | ||
| urma::UrmaEndpoint::PollingModeRelease(0); | ||
| urma::UrmaEndpoint endpoint(nullptr); | ||
| endpoint._cq_sid = 7; | ||
|
|
||
| endpoint.PollerAddCqSid(); | ||
| endpoint.PollerRemoveCqSid(); | ||
| for (auto& poller : urma::UrmaEndpoint::PollerGroups()[0].pollers) { | ||
| urma::UrmaEndpoint::CqSidOp op; | ||
| EXPECT_FALSE(poller.op_queue.Dequeue(op)); | ||
| } | ||
| } | ||
|
|
||
| INSTANTIATE_TEST_SUITE_P(BthreadAndPthread, UrmaPollingTest, | ||
| ::testing::Bool()); | ||
|
|
||
| namespace { | ||
| std::atomic<int> g_exit_pollers_released{0}; | ||
| urma::UrmaEndpoint* g_exit_endpoint = nullptr; | ||
|
|
||
| void CheckPollingExit() { | ||
| if (g_exit_pollers_released.load() != 2) { | ||
| std::_Exit(91); | ||
| } | ||
| // This handler was registered before URMA initialization, so it runs | ||
| // after the polling exit handler, like a late Socket/endpoint destructor. | ||
| delete g_exit_endpoint; | ||
| g_exit_endpoint = nullptr; | ||
| } | ||
| } // namespace | ||
|
|
||
| TEST(UrmaPollingDeathTest, global_shutdown_joins_all_tags) { | ||
| ::testing::FLAGS_gtest_death_test_style = "threadsafe"; | ||
| EXPECT_EXIT({ | ||
| urma::FLAGS_urma_use_polling = true; | ||
| urma::FLAGS_urma_poller_num = 2; | ||
| FLAGS_task_group_ntags = 2; | ||
| if (urma::UrmaEndpoint::GlobalInitialize() != 0 || | ||
| urma::UrmaEndpoint::PollingModeInitialize( | ||
| 0, nullptr, nullptr, | ||
| [] { ++g_exit_pollers_released; }) != 0) { | ||
| std::_Exit(92); | ||
| } | ||
| if (urma::UrmaEndpoint::PollingModeInitialize( | ||
| 1, nullptr, nullptr, | ||
| [] { ++g_exit_pollers_released; }) != 0) { | ||
| std::_Exit(92); | ||
| } | ||
| urma::UrmaEndpoint::GlobalPollingModeRelease(); | ||
| urma::UrmaEndpoint::GlobalPollingModeRelease(); | ||
| if (g_exit_pollers_released.load() != 4) { | ||
| std::_Exit(93); | ||
| } | ||
| std::_Exit(0); | ||
| }, ::testing::ExitedWithCode(0), ""); | ||
| } | ||
|
|
||
| TEST(UrmaPollingDeathTest, exit_stops_pollers_before_late_endpoint_cleanup) { | ||
| // Only the link-time mock can provide device resources independently of | ||
| // the host. Release the probe initialization before starting the child. | ||
| urma_init_attr_t init_attr{}; | ||
| const urma_status_t status = urma_init(&init_attr); | ||
| if (status != URMA_SUCCESS && status != URMA_EEXIST) { | ||
| GTEST_SKIP() << "Polling exit test requires the URMA link-time mock"; | ||
| } | ||
| int num_devices = 0; | ||
| urma_device_t** devices = urma_get_device_list(&num_devices); | ||
| const bool using_mock = devices && num_devices > 0 && | ||
| strcmp(devices[0]->name, "mock_urma_device") == 0; | ||
| if (devices) { | ||
| urma_free_device_list(devices); | ||
| } | ||
| if (status == URMA_SUCCESS) { | ||
| ASSERT_EQ(URMA_SUCCESS, urma_uninit()); | ||
| } | ||
| if (!using_mock) { | ||
| GTEST_SKIP() << "Polling exit test requires the URMA link-time mock"; | ||
| } | ||
| ::testing::FLAGS_gtest_death_test_style = "threadsafe"; | ||
| EXPECT_EXIT({ | ||
| urma::FLAGS_urma_use_polling = true; | ||
| urma::FLAGS_urma_poller_num = 2; | ||
| urma::FLAGS_urma_buffer_size = 4096; | ||
| urma::FLAGS_urma_buffer_count = 512; | ||
| urma::FLAGS_urma_sq_size = 16; | ||
| urma::FLAGS_urma_rq_size = 16; | ||
| urma::FLAGS_urma_prepared_jetty_cnt = 0; | ||
| urma::g_skip_urma_init = false; | ||
| if (std::atexit(CheckPollingExit) != 0) { | ||
| std::_Exit(94); | ||
| } | ||
| urma::GlobalUrmaInitializeOrDie(); | ||
| if (!urma::IsUrmaAvailable() || | ||
| urma::UrmaEndpoint::PollingModeInitialize( | ||
| 0, nullptr, nullptr, | ||
| [] { ++g_exit_pollers_released; }) != 0) { | ||
| std::_Exit(95); | ||
| } | ||
| g_exit_endpoint = new urma::UrmaEndpoint(nullptr); | ||
| g_exit_endpoint->_resource = new urma::UrmaResource; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [replied by brpc-oncall robot] False positive: |
||
| g_exit_endpoint->_cq_sid = 7; | ||
|
Comment on lines
+191
to
+193
|
||
| std::exit(0); | ||
| }, ::testing::ExitedWithCode(0), ""); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // v2 binary HelloMessage: serialize + deserialize round-trips. | ||
| // --------------------------------------------------------------------------- | ||
|
|
||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[replied by brpc-oncall robot] The exit path is still gated on
FLAGS_urma_use_polling:ReleasePollingModeAtExit()is registered only when the flag was true at init time, butPollingModeRelease()re-reads the flag's current value. If anything clears the flag before exit (application re-configuration, a test usingFlagSaver), the atexit handler silently skips the join and the ordering hazard this PR fixes comes back. The flag check is not needed for safety here becausetag >= poller_groups.size()already returns for the empty vector case; consider dropping it (at least for the global/exit path) so shutdown depends only ongroup.running.