From 997f6a65ba5b6b2e49c7485d8c4961f36313b98f Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Fri, 3 Feb 2023 20:17:53 +0200 Subject: [PATCH 1/2] semaphore_test: do not reassign units after they're destroyed The test case fails with g++ (GCC) 12.2.1 20221121 (Red Hat 12.2.1-4) ``` tests/unit/semaphore_test.cc(0): Entering test case "test_semaphore_units_splitting" src/testing/seastar_test.cc(43): info: check true has passed tests/unit/semaphore_test.cc(269): info: check units.count() == 3 has passed tests/unit/semaphore_test.cc(270): info: check sm.available_units() == 0 has passed tests/unit/semaphore_test.cc(272): info: check sm.available_units() == 0 has passed tests/unit/semaphore_test.cc(273): info: check units.count() == 2 has passed tests/unit/semaphore_test.cc(274): info: check split.count() == 1 has passed tests/unit/semaphore_test.cc(276): info: check sm.available_units() == 1 has passed tests/unit/semaphore_test.cc(279): fatal error: in "test_semaphore_units_splitting": critical check sm.available_units() == 0 has failed [2 != 0] ``` Avoid reassigning `units` after it is destroyed. Split the existing test sub-cases into test cases of their own. Signed-off-by: Benny Halevy --- tests/unit/semaphore_test.cc | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/tests/unit/semaphore_test.cc b/tests/unit/semaphore_test.cc index b62b6798795..3f94c0f60d7 100644 --- a/tests/unit/semaphore_test.cc +++ b/tests/unit/semaphore_test.cc @@ -262,7 +262,7 @@ SEASTAR_TEST_CASE(test_with_semaphore) { }); } -SEASTAR_THREAD_TEST_CASE(test_semaphore_units_splitting) { +SEASTAR_THREAD_TEST_CASE(test_semaphore_units_valid_splitting) { auto sm = semaphore(2); auto units = get_units(sm, 2, 1min).get0(); { @@ -272,25 +272,32 @@ SEASTAR_THREAD_TEST_CASE(test_semaphore_units_splitting) { BOOST_REQUIRE_EQUAL(sm.available_units(), 0); } BOOST_REQUIRE_EQUAL(sm.available_units(), 1); - units.~semaphore_units(); - units = get_units(sm, 2, 1min).get0(); +} + +SEASTAR_THREAD_TEST_CASE(test_semaphore_units_invalid_splitting) { + auto sm = semaphore(2); + auto units = get_units(sm, 2, 1min).get0(); BOOST_REQUIRE_EQUAL(sm.available_units(), 0); BOOST_REQUIRE_THROW(units.split(10), std::invalid_argument); BOOST_REQUIRE_EQUAL(sm.available_units(), 0); } -SEASTAR_THREAD_TEST_CASE(test_semaphore_units_return) { +SEASTAR_THREAD_TEST_CASE(test_semaphore_units_return_when_destroyed) { auto sm = semaphore(3); + { auto units = get_units(sm, 3, 1min).get0(); BOOST_REQUIRE_EQUAL(units.count(), 3); BOOST_REQUIRE_EQUAL(sm.available_units(), 0); BOOST_REQUIRE_EQUAL(units.return_units(1), 2); BOOST_REQUIRE_EQUAL(units.count(), 2); BOOST_REQUIRE_EQUAL(sm.available_units(), 1); - units.~semaphore_units(); + } BOOST_REQUIRE_EQUAL(sm.available_units(), 3); +} - units = get_units(sm, 2, 1min).get0(); +SEASTAR_THREAD_TEST_CASE(test_semaphore_units_return_all) { + auto sm = semaphore(3); + auto units = get_units(sm, 2, 1min).get0(); BOOST_REQUIRE_EQUAL(sm.available_units(), 1); BOOST_REQUIRE_THROW(units.return_units(10), std::invalid_argument); BOOST_REQUIRE_EQUAL(sm.available_units(), 1); From c4733e50ba0714e3ae4ee69c6e33a956cfce79ed Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Fri, 3 Feb 2023 18:27:00 +0200 Subject: [PATCH 2/2] semaphore: semaphore_units: return all units when reassigned When semaphore_units are (move-) reassigned with other units, the held units aren't currently returned to the semaphore. Call return_all before reassigning the semaphore_units object. Add a unit test reproducer. Fixes #1465 Signed-off-by: Benny Halevy --- include/seastar/core/semaphore.hh | 7 +++++-- tests/unit/semaphore_test.cc | 14 ++++++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/include/seastar/core/semaphore.hh b/include/seastar/core/semaphore.hh index ad3a0a18014..bf125413227 100644 --- a/include/seastar/core/semaphore.hh +++ b/include/seastar/core/semaphore.hh @@ -482,8 +482,11 @@ public: semaphore_units(semaphore_units&& o) noexcept : _sem(o._sem), _n(std::exchange(o._n, 0)) { } semaphore_units& operator=(semaphore_units&& o) noexcept { - _sem = o._sem; - _n = std::exchange(o._n, 0); + if (this != &o) { + return_all(); + _sem = o._sem; + _n = std::exchange(o._n, 0); + } return *this; } semaphore_units(const semaphore_units&) = delete; diff --git a/tests/unit/semaphore_test.cc b/tests/unit/semaphore_test.cc index 3f94c0f60d7..b9a15db7f60 100644 --- a/tests/unit/semaphore_test.cc +++ b/tests/unit/semaphore_test.cc @@ -438,3 +438,17 @@ SEASTAR_THREAD_TEST_CASE(test_semaphore_abort_before_wait) { BOOST_CHECK_THROW(fut1.get(), semaphore_aborted); BOOST_REQUIRE_EQUAL(x, 0); } + +SEASTAR_THREAD_TEST_CASE(test_reassigned_units_are_returned) { + auto sem0 = semaphore(1); + auto sem1 = semaphore(1); + auto units = get_units(sem0, 1).get(); + auto wait = sem0.wait(1); + BOOST_REQUIRE(!wait.available()); + units = get_units(sem1, 1).get(); + timer t([] { abort(); }); + t.arm(1s); + // will hang if units are not returned when reassigned + wait.get(); + t.cancel(); +}