From e77ee67bfacf9a0d3b9e7cd164531a2be158adc9 Mon Sep 17 00:00:00 2001 From: Fernando Sahmkow Date: Wed, 13 May 2020 13:49:36 -0400 Subject: [PATCH] Common/Fiber: Address Feedback and Correct Memory leaks. --- src/common/fiber.cpp | 58 ++++++++++++++++++++++++-------------------- src/common/fiber.h | 17 +++++++------ 2 files changed, 41 insertions(+), 34 deletions(-) diff --git a/src/common/fiber.cpp b/src/common/fiber.cpp index 6ea314d75..f97ad433b 100644 --- a/src/common/fiber.cpp +++ b/src/common/fiber.cpp @@ -21,7 +21,7 @@ struct Fiber::FiberImpl { LPVOID rewind_handle = nullptr; }; -void Fiber::start() { +void Fiber::Start() { ASSERT(previous_fiber != nullptr); previous_fiber->guard.unlock(); previous_fiber.reset(); @@ -29,7 +29,7 @@ void Fiber::start() { UNREACHABLE(); } -void Fiber::onRewind() { +void Fiber::OnRewind() { ASSERT(impl->handle != nullptr); DeleteFiber(impl->handle); impl->handle = impl->rewind_handle; @@ -38,14 +38,14 @@ void Fiber::onRewind() { UNREACHABLE(); } -void __stdcall Fiber::FiberStartFunc(void* fiber_parameter) { +void Fiber::FiberStartFunc(void* fiber_parameter) { auto fiber = static_cast(fiber_parameter); - fiber->start(); + fiber->Start(); } -void __stdcall Fiber::RewindStartFunc(void* fiber_parameter) { +void Fiber::RewindStartFunc(void* fiber_parameter) { auto fiber = static_cast(fiber_parameter); - fiber->onRewind(); + fiber->OnRewind(); } Fiber::Fiber(std::function&& entry_point_func, void* start_parameter) @@ -59,8 +59,11 @@ Fiber::Fiber() { } Fiber::~Fiber() { + if (released) { + return; + } // Make sure the Fiber is not being used - bool locked = guard.try_lock(); + const bool locked = guard.try_lock(); ASSERT_MSG(locked, "Destroying a fiber that's still running"); if (locked) { guard.unlock(); @@ -75,6 +78,7 @@ void Fiber::Exit() { } ConvertFiberToThread(); guard.unlock(); + released = true; } void Fiber::SetRewindPoint(std::function&& rewind_func, void* start_parameter) { @@ -89,22 +93,21 @@ void Fiber::Rewind() { SwitchToFiber(impl->rewind_handle); } -void Fiber::YieldTo(std::shared_ptr from, std::shared_ptr to) { +void Fiber::YieldTo(std::shared_ptr& from, std::shared_ptr& to) { ASSERT_MSG(from != nullptr, "Yielding fiber is null!"); ASSERT_MSG(to != nullptr, "Next fiber is null!"); to->guard.lock(); to->previous_fiber = from; SwitchToFiber(to->impl->handle); - auto previous_fiber = from->previous_fiber; - ASSERT(previous_fiber != nullptr); - previous_fiber->guard.unlock(); - previous_fiber.reset(); + ASSERT(from->previous_fiber != nullptr); + from->previous_fiber->guard.unlock(); + from->previous_fiber.reset(); } std::shared_ptr Fiber::ThreadToFiber() { std::shared_ptr fiber = std::shared_ptr{new Fiber()}; fiber->guard.lock(); - fiber->impl->handle = ConvertThreadToFiber(NULL); + fiber->impl->handle = ConvertThreadToFiber(nullptr); fiber->is_thread_fiber = true; return fiber; } @@ -120,7 +123,7 @@ struct Fiber::FiberImpl { boost::context::detail::fcontext_t rewind_context; }; -void Fiber::start(boost::context::detail::transfer_t& transfer) { +void Fiber::Start(boost::context::detail::transfer_t& transfer) { ASSERT(previous_fiber != nullptr); previous_fiber->impl->context = transfer.fctx; previous_fiber->guard.unlock(); @@ -129,7 +132,7 @@ void Fiber::start(boost::context::detail::transfer_t& transfer) { UNREACHABLE(); } -void Fiber::onRewind(boost::context::detail::transfer_t& [[maybe_unused]] transfer) { +void Fiber::OnRewind([[maybe_unused]] boost::context::detail::transfer_t& transfer) { ASSERT(impl->context != nullptr); impl->context = impl->rewind_context; impl->rewind_context = nullptr; @@ -142,17 +145,16 @@ void Fiber::onRewind(boost::context::detail::transfer_t& [[maybe_unused]] transf void Fiber::FiberStartFunc(boost::context::detail::transfer_t transfer) { auto fiber = static_cast(transfer.data); - fiber->start(transfer); + fiber->Start(transfer); } void Fiber::RewindStartFunc(boost::context::detail::transfer_t transfer) { auto fiber = static_cast(transfer.data); - fiber->onRewind(transfer); + fiber->OnRewind(transfer); } Fiber::Fiber(std::function&& entry_point_func, void* start_parameter) - : guard{}, entry_point{std::move(entry_point_func)}, start_parameter{start_parameter}, - previous_fiber{} { + : entry_point{std::move(entry_point_func)}, start_parameter{start_parameter} { impl = std::make_unique(); impl->stack_limit = impl->stack.data(); impl->rewind_stack_limit = impl->rewind_stack.data(); @@ -171,8 +173,11 @@ Fiber::Fiber() { } Fiber::~Fiber() { + if (released) { + return; + } // Make sure the Fiber is not being used - bool locked = guard.try_lock(); + const bool locked = guard.try_lock(); ASSERT_MSG(locked, "Destroying a fiber that's still running"); if (locked) { guard.unlock(); @@ -180,11 +185,13 @@ Fiber::~Fiber() { } void Fiber::Exit() { + ASSERT_MSG(is_thread_fiber, "Exitting non main thread fiber"); if (!is_thread_fiber) { return; } guard.unlock(); + released = true; } void Fiber::Rewind() { @@ -196,17 +203,16 @@ void Fiber::Rewind() { boost::context::detail::jump_fcontext(impl->rewind_context, this); } -void Fiber::YieldTo(std::shared_ptr from, std::shared_ptr to) { +void Fiber::YieldTo(std::shared_ptr& from, std::shared_ptr& to) { ASSERT_MSG(from != nullptr, "Yielding fiber is null!"); ASSERT_MSG(to != nullptr, "Next fiber is null!"); to->guard.lock(); to->previous_fiber = from; auto transfer = boost::context::detail::jump_fcontext(to->impl->context, to.get()); - auto previous_fiber = from->previous_fiber; - ASSERT(previous_fiber != nullptr); - previous_fiber->impl->context = transfer.fctx; - previous_fiber->guard.unlock(); - previous_fiber.reset(); + ASSERT(from->previous_fiber != nullptr); + from->previous_fiber->impl->context = transfer.fctx; + from->previous_fiber->guard.unlock(); + from->previous_fiber.reset(); } std::shared_ptr Fiber::ThreadToFiber() { diff --git a/src/common/fiber.h b/src/common/fiber.h index cab7bc4b5..dafc1100e 100644 --- a/src/common/fiber.h +++ b/src/common/fiber.h @@ -46,7 +46,7 @@ public: /// Yields control from Fiber 'from' to Fiber 'to' /// Fiber 'from' must be the currently running fiber. - static void YieldTo(std::shared_ptr from, std::shared_ptr to); + static void YieldTo(std::shared_ptr& from, std::shared_ptr& to); static std::shared_ptr ThreadToFiber(); void SetRewindPoint(std::function&& rewind_func, void* start_parameter); @@ -65,13 +65,13 @@ private: Fiber(); #if defined(_WIN32) || defined(WIN32) - void onRewind(); - void start(); + void OnRewind(); + void Start(); static void FiberStartFunc(void* fiber_parameter); static void RewindStartFunc(void* fiber_parameter); #else - void onRewind(boost::context::detail::transfer_t& transfer); - void start(boost::context::detail::transfer_t& transfer); + void OnRewind(boost::context::detail::transfer_t& transfer); + void Start(boost::context::detail::transfer_t& transfer); static void FiberStartFunc(boost::context::detail::transfer_t transfer); static void RewindStartFunc(boost::context::detail::transfer_t transfer); #endif @@ -79,13 +79,14 @@ private: struct FiberImpl; SpinLock guard{}; - std::function entry_point{}; - std::function rewind_point{}; + std::function entry_point; + std::function rewind_point; void* rewind_parameter{}; void* start_parameter{}; - std::shared_ptr previous_fiber{}; + std::shared_ptr previous_fiber; std::unique_ptr impl; bool is_thread_fiber{}; + bool released{}; }; } // namespace Common