From 414a637ebf5f0f3d00080701aacaf4da3644ed7b Mon Sep 17 00:00:00 2001 From: Riley Scheid Date: Tue, 7 Jul 2026 01:38:37 +0000 Subject: [PATCH 1/3] Wait for borrows to be relinquished before dest... ...roying the borrowed object --- include/stout/borrowed_ptr.h | 66 +++++++++++++++++- include/stout/stateful-tally.h | 11 +++ tests/borrowed_ptr.cc | 120 +++++++++++++++++++++++++++++++++ 3 files changed, 194 insertions(+), 3 deletions(-) diff --git a/include/stout/borrowed_ptr.h b/include/stout/borrowed_ptr.h index 3244696..b49e42a 100644 --- a/include/stout/borrowed_ptr.h +++ b/include/stout/borrowed_ptr.h @@ -92,6 +92,23 @@ class TypeErasedBorrowable { } } + // Transitions to 'Destructing', which prevents any new borrows from + // being taken, and then waits for all outstanding borrows to be + // relinquished. + // + // Types that derive from 'enable_borrowable_from_this' must call + // this at the beginning of their most-derived destructor: by the + // time '~enable_borrowable_from_this()' runs their members have + // already been destroyed, which is too late to wait for borrowers + // that might still be using them. + void DestructingAndWait() { + auto state = State::Borrowing; + if (!tally_.Update(state, State::Destructing)) { + LOG(FATAL) << "Unable to transition to Destructing from state " << state; + } + WaitUntilBorrowsEquals(0); + } + protected: TypeErasedBorrowable() : tally_(State::Borrowing) {} @@ -108,15 +125,17 @@ class TypeErasedBorrowable { virtual ~TypeErasedBorrowable() { auto state = State::Borrowing; - if (!tally_.Update(state, State::Destructing)) { - LOG(FATAL) << "Unable to transition to Destructing from state " << state; - } else { + if (tally_.Update(state, State::Destructing)) { // NOTE: it's possible that we'll block forever if exceptions // were thrown and destruction was not successful. // if (!std::uncaught_exceptions() > 0) { WaitUntilBorrowsEquals(0); // } + } else if (state != State::Destructing) { + LOG(FATAL) << "Unable to transition to Destructing from state " << state; } + // If the state is already 'Destructing' then 'DestructingAndWait()' + // has already run and all borrows have been relinquished. } enum class State : uint8_t { @@ -193,6 +212,14 @@ class Borrowable : public TypeErasedBorrowable { : TypeErasedBorrowable(std::move(that)), t_(std::move(that.t_)) {} + ~Borrowable() { + // Wait for all borrows to be relinquished before 't_', the object + // that they refer to, gets destroyed; the wait in + // '~TypeErasedBorrowable()' happens after members are destroyed, + // which is too late. + DestructingAndWait(); + } + borrowed_ref Borrow() { auto state = State::Borrowing; if (tally_.Increment(state)) { @@ -255,6 +282,14 @@ class Borrowable> : public TypeErasedBorrowable { : TypeErasedBorrowable(std::move(that)), t_(std::move(that.t_)) {} + ~Borrowable() { + // Wait for all borrows to be relinquished before 't_', and with it + // the object that they refer to, gets destroyed; the wait in + // '~TypeErasedBorrowable()' happens after members are destroyed, + // which is too late. + DestructingAndWait(); + } + borrowed_ref Borrow() { auto state = State::Borrowing; if (tally_.Increment(state)) { @@ -309,6 +344,31 @@ class Borrowable> : public TypeErasedBorrowable { template class enable_borrowable_from_this : public TypeErasedBorrowable { public: + // Declaring a destructor suppresses the implicitly-generated + // constructors, so default them to keep this type (and types + // deriving from it) copyable and moveable as before. + enable_borrowable_from_this() = default; + + enable_borrowable_from_this(const enable_borrowable_from_this& that) = + default; + + enable_borrowable_from_this(enable_borrowable_from_this&& that) = default; + + ~enable_borrowable_from_this() { + // Check the contract that the destructor of the derived type 'T' + // has already called 'DestructingAndWait()', i.e., that we have + // transitioned to 'Destructing' and that all borrows have been + // relinquished: the members of 'T' have already been destroyed by + // the time we get here, which is too late to wait for borrowers + // that might still be using them. + auto [state, count] = tally_.Load(); + CHECK(state == State::Destructing && count == 0) + << "The destructor of a type deriving from " + "'enable_borrowable_from_this' must call " + "'DestructingAndWait()' (state: " + << state << ", outstanding borrows: " << count << ")"; + } + borrowed_ref Borrow() { static_assert( std::is_base_of_v, T>, diff --git a/include/stout/stateful-tally.h b/include/stout/stateful-tally.h index 9c16845..e3ee7d4 100644 --- a/include/stout/stateful-tally.h +++ b/include/stout/stateful-tally.h @@ -30,6 +30,17 @@ struct StatefulTally { return S(value.load() >> ((sizeof(size_t) - 1) * 8)); } + // Returns the current (state, count) pair decoded from a single + // atomic load, i.e., a consistent snapshot of both. + std::pair Load() const { + size_t loaded = value.load(); + + size_t count = (loaded << 8) >> 8; + size_t state = loaded >> ((sizeof(size_t) - 1) * 8); + + return std::make_pair(S(state), count); + } + template std::pair Wait(Predicate&& predicate) { for (AtomicBackoff b;; b.pause()) { diff --git a/tests/borrowed_ptr.cc b/tests/borrowed_ptr.cc index 17c6f95..9230b83 100644 --- a/tests/borrowed_ptr.cc +++ b/tests/borrowed_ptr.cc @@ -1,6 +1,7 @@ #include "stout/borrowed_ptr.h" #include +#include #include #include #include @@ -422,6 +423,9 @@ TEST(BorrowTest, EnableBorrowableFromThis) { public: Foo(int i) : i(i) {} + ~Foo() { + DestructingAndWait(); + } int i = 0; }; @@ -442,6 +446,10 @@ TEST(BorrowTest, EnableBorrowableFromThisMove) { public: Foo(int i) : i(i) {} + Foo(Foo&& that) = default; + ~Foo() { + DestructingAndWait(); + } int i = 0; }; @@ -464,6 +472,9 @@ TEST(BorrowTest, EnableBorrowableFromThisCopy) { public: Foo(int i) : i(i) {} + ~Foo() { + DestructingAndWait(); + } int i = 0; }; @@ -479,3 +490,112 @@ TEST(BorrowTest, EnableBorrowableFromThisCopy) { EXPECT_EQ(borrowed->i, 42); } + + +TEST(BorrowTest, DestructWaitsForBorrows) { + auto* s = new Borrowable("hello world"); + + borrowed_ref borrowed = s->Borrow(); + + atomic destructed(false); + + thread t([&]() { + // Waits for 'borrowed' to be relinquished. + delete s; + destructed.store(true); + }); + + // The destructor must wait for our borrow to be relinquished + // before destroying the borrowed object, so this read is safe no + // matter how far 'delete' has gotten on the other thread. + EXPECT_EQ("hello world", *borrowed); + + EXPECT_FALSE(destructed.load()); + + { + borrowed_ref relinquished = std::move(borrowed); + } + + t.join(); + + EXPECT_TRUE(destructed.load()); +} + + +TEST(BorrowTest, DestructWaitsForBorrowsUniquePtr) { + auto* s = new Borrowable>( + std::make_unique("hello world")); + + borrowed_ref borrowed = s->Borrow(); + + atomic destructed(false); + + thread t([&]() { + // Waits for 'borrowed' to be relinquished. + delete s; + destructed.store(true); + }); + + // The destructor must wait for our borrow to be relinquished + // before destroying the borrowed object, so this read is safe no + // matter how far 'delete' has gotten on the other thread. + EXPECT_EQ("hello world", *borrowed); + + EXPECT_FALSE(destructed.load()); + + { + borrowed_ref relinquished = std::move(borrowed); + } + + t.join(); + + EXPECT_TRUE(destructed.load()); +} + + +TEST(BorrowTest, EnableBorrowableFromThisDestructWaitsForBorrows) { + class Foo : public enable_borrowable_from_this { + public: + Foo(int i) + : i(i) {} + ~Foo() { + DestructingAndWait(); + } + int i = 0; + }; + + auto* foo = new Foo(42); + + borrowed_ptr borrowed = foo->Borrow(); + + atomic destructed(false); + + thread t([&]() { + // 'DestructingAndWait()' waits for 'borrowed' to be relinquished. + delete foo; + destructed.store(true); + }); + + // 'Foo's destructor calls 'DestructingAndWait()' before its members + // are destroyed, so this read is safe no matter how far 'delete' + // has gotten on the other thread. + EXPECT_EQ(borrowed->i, 42); + + EXPECT_FALSE(destructed.load()); + + borrowed.relinquish(); + + t.join(); + + EXPECT_TRUE(destructed.load()); +} + + +TEST(BorrowDeathTest, EnableBorrowableFromThisDestructingAndWaitContract) { + class Foo : public enable_borrowable_from_this {}; + + // Destroying a type deriving from 'enable_borrowable_from_this' + // whose destructor did not call 'DestructingAndWait()' violates the + // contract checked by '~enable_borrowable_from_this()'. + EXPECT_DEATH({ Foo foo; }, "must call 'DestructingAndWait"); +} From 3c43c13a782b6159cc44ab1ff634dd5e0eb12667 Mon Sep 17 00:00:00 2001 From: Riley Scheid Date: Wed, 8 Jul 2026 04:01:20 +0000 Subject: [PATCH 2/3] Upgrade to Bazel 6.6.0 to fix CI on macOS Tahoe runners --- .bazeliskrc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.bazeliskrc b/.bazeliskrc index 89b6466..c108287 100644 --- a/.bazeliskrc +++ b/.bazeliskrc @@ -1 +1 @@ -USE_BAZEL_VERSION=6.5.0 +USE_BAZEL_VERSION=6.6.0 From 36d35b382d37f9ecb6de2354c02b94f8d9920bdd Mon Sep 17 00:00:00 2001 From: Riley Scheid Date: Wed, 8 Jul 2026 13:57:36 -0700 Subject: [PATCH 3/3] Code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: Benjamin Hindman --- include/stout/borrowed_ptr.h | 69 ++++++++++++------------------------ tests/borrowed_ptr.cc | 2 +- 2 files changed, 23 insertions(+), 48 deletions(-) diff --git a/include/stout/borrowed_ptr.h b/include/stout/borrowed_ptr.h index b49e42a..46be0f1 100644 --- a/include/stout/borrowed_ptr.h +++ b/include/stout/borrowed_ptr.h @@ -96,11 +96,12 @@ class TypeErasedBorrowable { // being taken, and then waits for all outstanding borrows to be // relinquished. // - // Types that derive from 'enable_borrowable_from_this' must call - // this at the beginning of their most-derived destructor: by the - // time '~enable_borrowable_from_this()' runs their members have - // already been destroyed, which is too late to wait for borrowers - // that might still be using them. + // Every type deriving from 'TypeErasedBorrowable' must call this in + // its destructor before its members are destroyed: by the time + // '~TypeErasedBorrowable()' runs those members have already been + // destroyed, which is too late to wait for borrowers that might + // still be using them ('~TypeErasedBorrowable()' checks this + // contract and fails fast if it has not been followed). void DestructingAndWait() { auto state = State::Borrowing; if (!tally_.Update(state, State::Destructing)) { @@ -124,18 +125,18 @@ class TypeErasedBorrowable { } virtual ~TypeErasedBorrowable() { - auto state = State::Borrowing; - if (tally_.Update(state, State::Destructing)) { - // NOTE: it's possible that we'll block forever if exceptions - // were thrown and destruction was not successful. - // if (!std::uncaught_exceptions() > 0) { - WaitUntilBorrowsEquals(0); - // } - } else if (state != State::Destructing) { - LOG(FATAL) << "Unable to transition to Destructing from state " << state; - } - // If the state is already 'Destructing' then 'DestructingAndWait()' - // has already run and all borrows have been relinquished. + // Check the contract that the destructor of the deriving type has + // already called 'DestructingAndWait()', i.e., that we have + // transitioned to 'Destructing' and that all borrows have been + // relinquished: the members of the deriving type have already + // been destroyed by the time we get here, which is too late to + // wait for borrowers that might still be using them. + auto [state, count] = tally_.Load(); + CHECK(state == State::Destructing && count == 0) + << "The destructor of a type deriving from " + "'TypeErasedBorrowable' must call 'DestructingAndWait()' " + "(state: " + << state << ", outstanding borrows: " << count << ")"; } enum class State : uint8_t { @@ -214,9 +215,8 @@ class Borrowable : public TypeErasedBorrowable { ~Borrowable() { // Wait for all borrows to be relinquished before 't_', the object - // that they refer to, gets destroyed; the wait in - // '~TypeErasedBorrowable()' happens after members are destroyed, - // which is too late. + // that they refer to, gets destroyed; we need to wait here because + // this is where the actual data gets destroyed, which is too late. DestructingAndWait(); } @@ -284,8 +284,8 @@ class Borrowable> : public TypeErasedBorrowable { ~Borrowable() { // Wait for all borrows to be relinquished before 't_', and with it - // the object that they refer to, gets destroyed; the wait in - // '~TypeErasedBorrowable()' happens after members are destroyed, + // the object that they refer to, gets destroyed; we need to wait here + // because this is where the actual data gets destroyed, // which is too late. DestructingAndWait(); } @@ -344,31 +344,6 @@ class Borrowable> : public TypeErasedBorrowable { template class enable_borrowable_from_this : public TypeErasedBorrowable { public: - // Declaring a destructor suppresses the implicitly-generated - // constructors, so default them to keep this type (and types - // deriving from it) copyable and moveable as before. - enable_borrowable_from_this() = default; - - enable_borrowable_from_this(const enable_borrowable_from_this& that) = - default; - - enable_borrowable_from_this(enable_borrowable_from_this&& that) = default; - - ~enable_borrowable_from_this() { - // Check the contract that the destructor of the derived type 'T' - // has already called 'DestructingAndWait()', i.e., that we have - // transitioned to 'Destructing' and that all borrows have been - // relinquished: the members of 'T' have already been destroyed by - // the time we get here, which is too late to wait for borrowers - // that might still be using them. - auto [state, count] = tally_.Load(); - CHECK(state == State::Destructing && count == 0) - << "The destructor of a type deriving from " - "'enable_borrowable_from_this' must call " - "'DestructingAndWait()' (state: " - << state << ", outstanding borrows: " << count << ")"; - } - borrowed_ref Borrow() { static_assert( std::is_base_of_v, T>, diff --git a/tests/borrowed_ptr.cc b/tests/borrowed_ptr.cc index 9230b83..596ae0d 100644 --- a/tests/borrowed_ptr.cc +++ b/tests/borrowed_ptr.cc @@ -596,6 +596,6 @@ TEST(BorrowDeathTest, EnableBorrowableFromThisDestructingAndWaitContract) { // Destroying a type deriving from 'enable_borrowable_from_this' // whose destructor did not call 'DestructingAndWait()' violates the - // contract checked by '~enable_borrowable_from_this()'. + // contract checked by '~TypeErasedBorrowable()'. EXPECT_DEATH({ Foo foo; }, "must call 'DestructingAndWait"); }