Merge 'semaphore: semaphore_units: return units when reassigned' from…

… Benny Halevy When semaphore_units are (move-) reassigned with other units, the held units aren't currently returned to the semaphore. This change implements the move-reassignment operator by swapping the semaphore and the units counter so that when the moved-from semaphore_units object is destroyed, the units will be returned to the semaphore via `~semaphore_units` -> `return_all`. Add a unit test reproducer. Fixes #1465 Signed-off-by: Benny Halevy <[email protected]> Closes #1466 * github.com:scylladb/seastar: semaphore: semaphore_units: return all units when reassigned semaphore_test: do not reassign units after they're destroyed
scylladb · Jul 25, 2023 · c204fe7 · c204fe7
2 parents 19c56e1 + c4733e5
commit c204fe7
Show file tree

Hide file tree

Showing 2 changed files with 32 additions and 8 deletions.
diff --git 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
@@ -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);
@@ -431,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();
+}