diff --git a/source/common/common/BUILD b/source/common/common/BUILD index 155bace5e38be..100cde3e59caf 100644 --- a/source/common/common/BUILD +++ b/source/common/common/BUILD @@ -96,6 +96,15 @@ envoy_cc_library( hdrs = ["c_smart_ptr.h"], ) +envoy_cc_library( + name = "callbacks_handler_lib", + hdrs = ["callbacks_handler.h"], + deps = [ + ":assert_lib", + "//envoy/common:optref_lib", + ], +) + envoy_cc_library( name = "cleanup_lib", hdrs = ["cleanup.h"], diff --git a/source/common/common/callbacks_handler.h b/source/common/common/callbacks_handler.h new file mode 100644 index 0000000000000..38f2e0c438fd6 --- /dev/null +++ b/source/common/common/callbacks_handler.h @@ -0,0 +1,189 @@ +#pragma once + +#include "envoy/common/optref.h" + +#include "source/common/common/assert.h" + +namespace Envoy { +namespace Common { + +/** + * A self-managing, move-able pair of non-owning back-references used to make async callbacks + * safe against either side being destroyed first, WITHOUT requiring the callbacks object to be + * owned by a shared_ptr (so no std::weak_ptr is available) and WITHOUT a heap allocation. + * + * The pair is: + * - CallbacksHandler : typically owned/embedded by the entity that issues the async request + * and is interested in the result. Its lifetime gates whether the callbacks are still "live". + * - CallbacksHandler::CallbacksWrapper : the token handed to the async machinery (stored in a + * std::function, a member, a container, ...). The async code calls wrapper.callbacks() and, + * only if it still has_value(), invokes the callbacks. + * + * Each side holds an OptRef to the other. When either side is reset() or destroyed it nulls out + * the peer's back-reference, so neither can ever dangle: + * - Destroying the handler clears the wrapper's callbacks_, so wrapper.callbacks() becomes empty + * and the async side learns the requester is gone. + * - Destroying the wrapper clears the handler's pointer to it, so the handler's own destruction + * is a no-op. + * + * The cycle is broken by onPeerReset(): the side that initiates teardown clears its own pointer + * FIRST and then calls the peer's onPeerReset(), which only nulls fields and never calls back. + * Hence no infinite recursion and no use-after-clear. + * + * Establishing a pair (the handler must out-exist the wrapper's construction call): + * CallbacksHandler handler; + * CallbacksWrapper wrapper(handler, my_callbacks); // wires both directions + * // hand `wrapper` (by move) to the async operation; keep `handler` near `my_callbacks`. + * + * Threading: NOT thread-safe. Both ends must live on, and be mutated from, the same thread + * (e.g. the same dispatcher). If the async completion can fire from another thread, this is the + * wrong tool -- use ThreadSafeCallbackManager instead. + * + * Copy/move: these are move-only types. Copy is explicitly deleted -- copying would alias the + * single back-reference the peer holds and immediately dangle. Move is defined to re-point the + * peer at the new address, which is what lets callers keep the pair on the stack / inline instead + * of paying for a heap allocation. + * + * Extending the classes: CallbacksHandler::reset()/onPeerReset() are virtual so a subclass can + * observe teardown. IMPORTANT for extenders -- to stay move-able (the whole point of this type), a + * subclass MUST NOT hand-declare a destructor or any copy/move operation, because doing so + * suppresses the implicitly-generated move operations and, since copy is deleted, leaves the + * subclass immovable. Put cleanup in an override of reset()/onPeerReset(), not in a destructor. + * Note the base destructor calls CallbacksHandler::reset() non-virtually, so a subclass override of + * reset() does NOT run during base destruction -- override onPeerReset() (called when the peer + * tears the link down) or do per-instance cleanup in member destruction order instead. + */ +template class CallbacksHandler { +public: + class CallbacksWrapper { + public: + CallbacksWrapper(const CallbacksWrapper&) = delete; + CallbacksWrapper& operator=(const CallbacksWrapper&) = delete; + + CallbacksWrapper(CallbacksHandler& handler, CallbacksType& callbacks) + : handler_(handler), callbacks_(callbacks) { + ASSERT(!handler.wrapper_.has_value(), "CallbacksHandler already has a wrapper"); + handler.wrapper_ = OptRef(*this); + } + + CallbacksWrapper(CallbacksWrapper&& to_move) noexcept { moveFrom(to_move); } + + CallbacksWrapper& operator=(CallbacksWrapper&& to_move) noexcept { + if (this != &to_move) { + // Detach our current handler (clears its back-pointer) and our callbacks before adopting. + reset(); + moveFrom(to_move); + } + return *this; + } + + ~CallbacksWrapper() { CallbacksWrapper::reset(); } + + /** + * @return an OptRef to the callbacks, empty if the handler has been destroyed or reset. The + * async side should call this and only invoke the callbacks if has_value() is true. + */ + OptRef callbacks() const { return callbacks_; } + + /** + * Actively tears down this side of the link: forgets the callbacks and tells the handler to + * drop its pointer to this wrapper. Idempotent. + */ + void reset() { + callbacks_.reset(); + auto handler = handler_; + handler_.reset(); + + if (handler.has_value()) { + handler->onPeerReset(); + } + } + + private: + // Re-points `to_move`'s handler back at this wrapper, adopts its link, and empties the source. + // Shared by the move constructor and move assignment; assumes any link previously held on this + // side is already gone (the constructor starts empty, the assignment calls reset() first). + void moveFrom(CallbacksWrapper& to_move) noexcept { + if (to_move.handler_.has_value()) { + to_move.handler_->wrapper_ = OptRef(*this); + } + handler_ = to_move.handler_; + callbacks_ = to_move.callbacks_; + to_move.handler_.reset(); + to_move.callbacks_.reset(); + } + + protected: + // Called BY the handler when the handler is the one tearing the link down. Only nulls fields; + // never calls back into the handler (that is what breaks the teardown cycle). + void onPeerReset() { + handler_.reset(); + callbacks_.reset(); + } + + // The handler needs access to onPeerReset()/handler_ on wrapper instances other than the one + // it encloses. (The reverse direction needs no friend: a nested class already has access to its + // enclosing class's members.) + friend class CallbacksHandler; + + OptRef> handler_; + OptRef callbacks_; + }; + + CallbacksHandler(const CallbacksHandler&) = delete; + CallbacksHandler& operator=(const CallbacksHandler&) = delete; + + CallbacksHandler() = default; + + CallbacksHandler(CallbacksHandler&& to_move) noexcept { moveFrom(to_move); } + + CallbacksHandler& operator=(CallbacksHandler&& to_move) noexcept { + if (this != &to_move) { + // Virtual: runs any subclass teardown, then drops our current wrapper's back-pointer. + reset(); + moveFrom(to_move); + } + return *this; + } + + // Virtual destructor calls reset() NON-virtually (qualified) so it always runs this class's + // teardown regardless of any subclass override; see the class comment for the implication. + virtual ~CallbacksHandler() { CallbacksHandler::reset(); } + + /** + * Actively tears down this side of the link: tells the wrapper to forget its callbacks (so the + * async side sees callbacks() as empty) and drops our pointer to it. Idempotent. Virtual so a + * subclass can observe/extend teardown. + */ + virtual void reset() { + auto wrapper = wrapper_; + wrapper_.reset(); + + if (wrapper.has_value()) { + wrapper->onPeerReset(); + } + } + +private: + // Re-points `to_move`'s wrapper back at this handler, adopts its link, and empties the source. + // Shared by the move constructor and move assignment; assumes any link previously held on this + // side is already gone (the constructor starts empty, the assignment calls reset() first). + void moveFrom(CallbacksHandler& to_move) noexcept { + if (to_move.wrapper_.has_value()) { + to_move.wrapper_->handler_ = OptRef(*this); + } + wrapper_ = to_move.wrapper_; + to_move.wrapper_.reset(); + } + +protected: + // Called BY the wrapper when the wrapper is the one tearing the link down. Only nulls our + // pointer; never calls back into the wrapper. Virtual so a subclass can observe peer-initiated + // teardown (this is the hook that DOES run when the wrapper is destroyed first). + virtual void onPeerReset() { wrapper_.reset(); } + + OptRef wrapper_; +}; + +} // namespace Common +} // namespace Envoy diff --git a/test/common/common/BUILD b/test/common/common/BUILD index 690a4b3f6a153..e79b8bd1108ef 100644 --- a/test/common/common/BUILD +++ b/test/common/common/BUILD @@ -153,6 +153,13 @@ envoy_cc_fuzz_test( deps = ["//source/common/common:hash_lib"], ) +envoy_cc_test( + name = "callbacks_handler_test", + srcs = ["callbacks_handler_test.cc"], + rbe_pool = "6gig", + deps = ["//source/common/common:callbacks_handler_lib"], +) + envoy_cc_test( name = "cleanup_test", srcs = ["cleanup_test.cc"], diff --git a/test/common/common/callbacks_handler_test.cc b/test/common/common/callbacks_handler_test.cc new file mode 100644 index 0000000000000..abfd67e01b9ed --- /dev/null +++ b/test/common/common/callbacks_handler_test.cc @@ -0,0 +1,248 @@ +#include +#include + +#include "source/common/common/callbacks_handler.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace Common { +namespace { + +struct TestCallbacks { + int id{0}; +}; + +using Handler = CallbacksHandler; +using Wrapper = Handler::CallbacksWrapper; + +// These are move-only types: move enabled (to keep them on the stack), copy deleted (it would +// alias the peer's single back-reference). +static_assert(std::is_move_constructible_v); +static_assert(std::is_move_assignable_v); +static_assert(!std::is_copy_constructible_v); +static_assert(!std::is_copy_assignable_v); +static_assert(std::is_move_constructible_v); +static_assert(std::is_move_assignable_v); +static_assert(!std::is_copy_constructible_v); +static_assert(!std::is_copy_assignable_v); + +// Construction wires both directions and exposes the callbacks. +TEST(CallbacksHandlerTest, BasicLinkAndAccess) { + TestCallbacks cbs{42}; + Handler handler; + Wrapper wrapper(handler, cbs); + + ASSERT_TRUE(wrapper.callbacks().has_value()); + EXPECT_EQ(&cbs, wrapper.callbacks().ptr()); + EXPECT_EQ(42, wrapper.callbacks()->id); +} + +// Destroying the handler first invalidates the wrapper; the wrapper survives with empty +// callbacks(). +TEST(CallbacksHandlerTest, DestroyHandlerFirst) { + TestCallbacks cbs; + Wrapper* wrapper_ptr = nullptr; + std::optional wrapper; + { + Handler handler; + wrapper.emplace(handler, cbs); + wrapper_ptr = &wrapper.value(); + EXPECT_TRUE(wrapper_ptr->callbacks().has_value()); + } + // Handler gone: wrapper must now be invalid (and not dangling). + EXPECT_FALSE(wrapper_ptr->callbacks().has_value()); + EXPECT_FALSE(wrapper_ptr->callbacks().has_value()); +} + +// Destroying the wrapper first leaves the handler safe to reset and destroy. +TEST(CallbacksHandlerTest, DestroyWrapperFirst) { + TestCallbacks cbs; + Handler handler; + { + Wrapper wrapper(handler, cbs); + EXPECT_TRUE(wrapper.callbacks().has_value()); + } + // Wrapper gone: handler is unlinked. reset() and destruction must be safe no-ops. + handler.reset(); + SUCCEED(); +} + +// Explicit reset() from the handler side invalidates the wrapper. +TEST(CallbacksHandlerTest, ExplicitHandlerReset) { + TestCallbacks cbs; + Handler handler; + Wrapper wrapper(handler, cbs); + + handler.reset(); + EXPECT_FALSE(wrapper.callbacks().has_value()); + // Double reset / destruction are safe. + handler.reset(); +} + +// Explicit reset() from the wrapper side unlinks the handler. +TEST(CallbacksHandlerTest, ExplicitWrapperReset) { + TestCallbacks cbs; + Handler handler; + Wrapper wrapper(handler, cbs); + + wrapper.reset(); + EXPECT_FALSE(wrapper.callbacks().has_value()); + // Handler no longer points at the wrapper; destroying it is a no-op. +} + +// Move-constructing the wrapper transfers the link to the new object and empties the source. +TEST(CallbacksHandlerTest, MoveConstructWrapper) { + TestCallbacks cbs{7}; + std::optional handler; + handler.emplace(); + + Wrapper src(*handler, cbs); + Wrapper dst(std::move(src)); + + // NOLINTNEXTLINE(bugprone-use-after-move) -- intentionally asserting the moved-from state. + EXPECT_FALSE(src.callbacks().has_value()); + EXPECT_TRUE(dst.callbacks().has_value()); + EXPECT_EQ(&cbs, dst.callbacks().ptr()); + + // The handler is now linked to dst: destroying it invalidates dst, not the empty src. + handler.reset(); + EXPECT_FALSE(dst.callbacks().has_value()); +} + +// Move-constructing the handler transfers the back-link so the wrapper still tracks it. +TEST(CallbacksHandlerTest, MoveConstructHandler) { + TestCallbacks cbs; + std::optional wrapper; + + Handler src; + // Build the wrapper against src, then move src into dst. + wrapper.emplace(src, cbs); + Handler dst(std::move(src)); + + EXPECT_TRUE(wrapper->callbacks().has_value()); + // Destroying the moved-from src must NOT invalidate the wrapper (the link moved to dst). + { + Handler sink(std::move(dst)); // move again to prove the link keeps following the handler. + EXPECT_TRUE(wrapper->callbacks().has_value()); + } + // sink destroyed -> wrapper invalidated. + EXPECT_FALSE(wrapper->callbacks().has_value()); +} + +// Self move-assignment is a no-op and keeps the link intact. +TEST(CallbacksHandlerTest, MoveAssignWrapperSelf) { + TestCallbacks cbs{99}; + Handler handler; + Wrapper wrapper(handler, cbs); + + Wrapper* alias = &wrapper; + wrapper = std::move(*alias); // self move-assign via alias to dodge the self-move warning. + + EXPECT_TRUE(wrapper.callbacks().has_value()); + EXPECT_EQ(99, wrapper.callbacks()->id); +} + +// Move-assigning over a live link detaches the old handler and adopts the source's link. +TEST(CallbacksHandlerTest, MoveAssignWrapperOverLiveLink) { + TestCallbacks cbs1{1}; + TestCallbacks cbs2{2}; + std::optional h1; + std::optional h2; + h1.emplace(); + h2.emplace(); + + Wrapper w1(*h1, cbs1); + Wrapper w2(*h2, cbs2); + + w1 = std::move(w2); + + ASSERT_TRUE(w1.callbacks().has_value()); + EXPECT_EQ(&cbs2, w1.callbacks().ptr()); + EXPECT_EQ(2, w1.callbacks()->id); + // NOLINTNEXTLINE(bugprone-use-after-move) + EXPECT_FALSE(w2.callbacks().has_value()); + + // h1 was detached: destroying it must not touch w1. + h1.reset(); + EXPECT_TRUE(w1.callbacks().has_value()); + + // Destroying h2 invalidates w1. + h2.reset(); + EXPECT_FALSE(w1.callbacks().has_value()); +} + +// Symmetric move-assign test from the handler side. +TEST(CallbacksHandlerTest, MoveAssignHandlerOverLiveLink) { + TestCallbacks cbs1{1}; + TestCallbacks cbs2{2}; + std::optional w1; + std::optional w2; + + Handler h1; + Handler h2; + w1.emplace(h1, cbs1); + w2.emplace(h2, cbs2); + + h1 = std::move(h2); + + // h1 now owns the link to w2; w1's handler (old h1) has been detached -> w1 invalid. + EXPECT_FALSE(w1->callbacks().has_value()); + EXPECT_TRUE(w2->callbacks().has_value()); + + // Destroying h1 (which now drives w2) invalidates w2. + h1.reset(); + EXPECT_FALSE(w2->callbacks().has_value()); +} + +// A subclass can observe peer-initiated teardown by overriding onPeerReset(), and active +// teardown by overriding reset(). Stays move-able because it declares no dtor/copy/move. +class ObservingHandler : public Handler { +public: + void reset() override { + ++reset_calls_; + Handler::reset(); + } + int reset_calls_{0}; + int peer_reset_calls_{0}; + +protected: + void onPeerReset() override { + ++peer_reset_calls_; + Handler::onPeerReset(); + } +}; + +TEST(CallbacksHandlerTest, SubclassObservesTeardown) { + TestCallbacks cbs; + ObservingHandler handler; + { + Wrapper wrapper(handler, cbs); + EXPECT_TRUE(wrapper.callbacks().has_value()); + // Wrapper destroyed here -> handler->onPeerReset() dispatches to the override. + } + EXPECT_EQ(1, handler.peer_reset_calls_); + EXPECT_EQ(0, handler.reset_calls_); + + // Explicit reset() dispatches to the override too. + ObservingHandler handler2; + TestCallbacks cbs2; + Wrapper wrapper2(handler2, cbs2); + handler2.reset(); + EXPECT_EQ(1, handler2.reset_calls_); + EXPECT_FALSE(wrapper2.callbacks().has_value()); +} + +// Attaching a second wrapper to a handler that already has one trips the ASSERT (debug builds). +TEST(CallbacksHandlerDeathTest, DoubleAttachAsserts) { + TestCallbacks cbs1; + TestCallbacks cbs2; + Handler handler; + Wrapper wrapper(handler, cbs1); + + EXPECT_DEBUG_DEATH({ Wrapper second(handler, cbs2); }, "already has a wrapper"); +} + +} // namespace +} // namespace Common +} // namespace Envoy