From 2207f9dd2c43b53d7ee7d95e77dd85ed935cb70d Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Fri, 22 Nov 2024 01:52:11 +0100 Subject: [PATCH 1/2] Defuse throttled_func when it's accidentally engaged --- src/cascadia/WinRTUtils/inc/ThrottledFunc.h | 2 +- src/inc/til/throttled_func.h | 31 +++++++++++++++------ 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/src/cascadia/WinRTUtils/inc/ThrottledFunc.h b/src/cascadia/WinRTUtils/inc/ThrottledFunc.h index 40482faf192..94c42b3362a 100644 --- a/src/cascadia/WinRTUtils/inc/ThrottledFunc.h +++ b/src/cascadia/WinRTUtils/inc/ThrottledFunc.h @@ -113,7 +113,7 @@ class ThrottledFunc : public std::enable_shared_from_this_func, self->_storage.take()); + self->_storage.apply(self->_func); } CATCH_LOG(); } diff --git a/src/inc/til/throttled_func.h b/src/inc/til/throttled_func.h index d250f475af5..bcf8cc78c68 100644 --- a/src/inc/til/throttled_func.h +++ b/src/inc/til/throttled_func.h @@ -30,12 +30,23 @@ namespace til } } - std::tuple take() + void apply(const auto& func) { - std::unique_lock guard{ _lock }; - auto pendingRunArgs = std::move(*_pendingRunArgs); - _pendingRunArgs.reset(); - return pendingRunArgs; + decltype(_pendingRunArgs) args; + { + std::unique_lock guard{ _lock }; + args = std::move(_pendingRunArgs); + _pendingRunArgs.reset(); + } + // Theoretically it should always have a value, because the throttled_func + // should not call the callback without there being a reason. + // But in practice a failure here was observed at least once. + // It's unknown to me what caused it, so the best we can do is avoid a crash. + assert(args.has_value()); + if (args) + { + std::apply(func, *args); + } } explicit operator bool() const @@ -60,10 +71,12 @@ namespace til return _isPending.exchange(true, std::memory_order_relaxed); } - std::tuple<> take() + void apply(const auto& func) { - reset(); - return {}; + if (_isPending.exchange(false, std::memory_order_relaxed)) + { + func(); + } } void reset() @@ -193,7 +206,7 @@ namespace til } else { - std::apply(_func, _storage.take()); + _storage.apply(_func); } } From 0c71d650db4ff64452931761316a52207160e0b5 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Mon, 25 Nov 2024 23:58:04 +0100 Subject: [PATCH 2/2] Address feedback, Some mild size reduction --- src/inc/til/throttled_func.h | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/src/inc/til/throttled_func.h b/src/inc/til/throttled_func.h index bcf8cc78c68..7a2d544add3 100644 --- a/src/inc/til/throttled_func.h +++ b/src/inc/til/throttled_func.h @@ -35,8 +35,7 @@ namespace til decltype(_pendingRunArgs) args; { std::unique_lock guard{ _lock }; - args = std::move(_pendingRunArgs); - _pendingRunArgs.reset(); + args = std::exchange(_pendingRunArgs, std::nullopt); } // Theoretically it should always have a value, because the throttled_func // should not call the callback without there being a reason. @@ -184,31 +183,24 @@ namespace til void flush() { WaitForThreadpoolTimerCallbacks(_timer.get(), true); - if (_storage) - { - _trailing_edge(); - } + _timer_callback(nullptr, this, nullptr); } private: static void __stdcall _timer_callback(PTP_CALLBACK_INSTANCE /*instance*/, PVOID context, PTP_TIMER /*timer*/) noexcept try { - static_cast(context)->_trailing_edge(); - } - CATCH_LOG() - - void _trailing_edge() - { + const auto self = static_cast(context); if constexpr (Leading) { - _storage.reset(); + self->_storage.reset(); } else { - _storage.apply(_func); + self->_storage.apply(self->_func); } } + CATCH_LOG() wil::unique_threadpool_timer _createTimer() {