From 61336aac9c5d7f447f8984bdd8733ac9b0d3e99b Mon Sep 17 00:00:00 2001 From: Raymond Chen Date: Thu, 7 Oct 2021 12:48:12 -0700 Subject: [PATCH] Allow an object to hold a weak reference to itself in a member Holding a weak reference to yourself is important when you have asynchronous callbacks that might race against the destructor. To avoid the race, you create a weak reference to yourself, and the callback function tries to upgrade the weak reference to a strong reference. * If it succeeds, then the strong reference prevents the object from destructing until the callback completes. * If it fails, then destruction has begun and the callback doesn't try to use the partially-destructed object. Full example is given below. Prior to this change, you could not have a weak reference to yourself because the `winrt::weak_ref` required that T be a complete type, which it won't be in the case that you are trying to use a weak reference as a member variable type. The fix is to move the `com_ref` out of the function prototypes and into the bodies. By the time the bodies are expanded, the type `T` will be complete, and everything will work. This is easy to do with `get()` since we can just make the function return `auto`. This is harder to do with the constructor. The trick is to make the constructor accept a universal reference, and then convert that universal reference to the desired `com_ref` in the body. There are several wrinkles to this. 1. The universal reference template type parameter has a default type of `com_ref const&`. This permits the passing of an anonymous braced parameter list to construct a `com_ref`. 2. We must use `std::is_convertible` to detect whether the `U&&` can be converted to a `com_ref const&`, rather than writing `com_ref const& ref = object;` because the latter is an explicit conversion, but parameter passing uses only implicit conversions. 3. The `std::is_convertible` test must be performed as part of SFINAE, so that `std::is_constructible, U>` returns the correct value. (If we did it as a `static_assert`, then `std::is_constructible, U>` would always be true, even though not all choices for `U` successfully compile.) 4. To avoid code size explosion, we factor the `com_ref` into a helper function `from_com_ref`, so that the conversion from `U&&` to `com_ref const&` happens outside the helper function, and the helper function itself can be shared. 5. We have to make sure that the universal reference constructor does not participate in overload resolution if the source is another `weak_ref`. Otherwise, that would interfere with the default copy/move constructors and default copy/move assignment operators. Fortunately, `weak_ref` is not implicitly convertible to `com_ref`, so the `is_convertible` SFINAE covers this already. Added tests to validate behavior, particularly that we didn't mess up the default copy/move constructors and assignments. Example ======= ```cpp // UnregisterWidgetCallback blocks until all outstanding // callbacks have returned. using unique_widget_callback = wil::unique_any; struct MyClass : implements { unique_widget_callback h; MyClass() { h.reset(RegisterWidgetCallback(s_OnWidgetCallback, this)); } void s_OnWidgetCallback(void* context) { ((MyClass*)context)->OnWidgetCallback(); } fire_and_forget OnWidgetCallback() { // take a strong reference to prevent // premature destruction auto strongThis = get_strong(); co_await resume_background(); do_stuff(); } }; ``` This code doesn't work, because there is a race condition if the widget callback occurs just after MyClass has started destructing but before it calls UnregisterWidgetCallback. In that case, the callback function will call `get_strong()` on a `MyClass` that has already started destructing. The hope was that the strong reference would prevent premature destruction, but we're **too late**: Destruction has already begun. You can't undestruct! (The call to `get_strong()` succeeds, but the thing you get back will not prevent destruction.) The solution is to use a `weak_ref`, because weak references expire *before* the destructor begins. In the callback, we try to promote the weak reference to a strong reference, and it will fail if destruction has begun. ```cpp struct MyClass : implements { // Order of declaration is important here. // weak_self must be declared first so it destructs last. weak_ref weak_self = get_weak(); unique_widget_callback h; MyClass() { h.reset(RegisterWidgetCallback(s_OnWidgetCallback, this)); } void s_OnWidgetCallback(void* context) { auto strongThis = ((MyClass*)context)->weak_self.get(); if (strongThis) strongThis->OnWidgetCallback(); } fire_and_forget OnWidgetCallback() { // take a strong reference to prevent // premature destruction auto strongThis = get_strong(); co_await resume_background(); do_stuff(); } }; ``` If destruction of `MyClass` has begun, the `weak_self.get()` will fail, and `s_OnWidgetCallback` will just return without doing anything. This solution requires that it be possible to create a `weak_ref` to a type that is still being defined. That's what this change enables. In the absence of this change, we have to use the default interface, which is more cumbersome and error-prone. ```cpp weak_ref> weak_self{ *this }; ``` and ```cpp auto strongThis = ((MyClass*)context)->weak_self.get(); if (strongThis) get_self(strongThis)->OnWidgetCallback(); ``` --- strings/base_weak_ref.h | 43 +++++++++++-------- test/old_tests/UnitTests/weak.cpp | 69 +++++++++++++++++++++++++++++++ 2 files changed, 94 insertions(+), 18 deletions(-) diff --git a/strings/base_weak_ref.h b/strings/base_weak_ref.h index 24fe8fb91..c98eb93a7 100644 --- a/strings/base_weak_ref.h +++ b/strings/base_weak_ref.h @@ -6,28 +6,17 @@ WINRT_EXPORT namespace winrt { weak_ref(std::nullptr_t = nullptr) noexcept {} - weak_ref(impl::com_ref const& object) + template const&, typename = std::enable_if_t const&>>> + weak_ref(U&& object) { - if (object) - { - if constexpr(impl::is_implements_v) - { - m_ref = std::move(object->get_weak().m_ref); - } - else - { - // An access violation (crash) on the following line means that the object does not support weak references. - // Avoid using weak_ref/auto_revoke with such objects. - check_hresult(object.template try_as()->GetWeakReference(m_ref.put())); - } - } + from_com_ref(static_cast const&>(object)); } - [[nodiscard]] impl::com_ref get() const noexcept + [[nodiscard]] auto get() const noexcept { if (!m_ref) { - return nullptr; + return impl::com_ref{ nullptr }; } if constexpr(impl::is_implements_v) @@ -36,13 +25,13 @@ WINRT_EXPORT namespace winrt m_ref->Resolve(guid_of(), put_abi(temp)); void* result = get_self(temp); detach_abi(temp); - return { result, take_ownership_from_abi }; + return impl::com_ref{ result, take_ownership_from_abi }; } else { void* result{}; m_ref->Resolve(guid_of(), &result); - return { result, take_ownership_from_abi }; + return impl::com_ref{ result, take_ownership_from_abi }; } } @@ -58,6 +47,24 @@ WINRT_EXPORT namespace winrt private: + template + void from_com_ref(U&& object) + { + if (object) + { + if constexpr (impl::is_implements_v) + { + m_ref = std::move(object->get_weak().m_ref); + } + else + { + // An access violation (crash) on the following line means that the object does not support weak references. + // Avoid using weak_ref/auto_revoke with such objects. + check_hresult(object.template try_as()->GetWeakReference(m_ref.put())); + } + } + } + com_ptr m_ref; }; diff --git a/test/old_tests/UnitTests/weak.cpp b/test/old_tests/UnitTests/weak.cpp index 650b6ae0f..b253e6010 100644 --- a/test/old_tests/UnitTests/weak.cpp +++ b/test/old_tests/UnitTests/weak.cpp @@ -49,6 +49,25 @@ namespace return L"WeakNoModuleLock"; } }; + + struct WeakWithSelfReference : implements + { + winrt::weak_ref weak_self = get_weak(); + + hstring ToString() + { + // Verify that the weak reference works as long as the object is alive. + REQUIRE(weak_self.get().get() == this); + + return L"WeakWithSelfReference"; + } + + ~WeakWithSelfReference() + { + // Verify that the weak reference cannot be resolved once destruction begins. + REQUIRE(weak_self.get() == nullptr); + } + }; } TEST_CASE("weak,source") @@ -313,6 +332,49 @@ TEST_CASE("weak,comparison") REQUIRE(refA1 != refNothing); } +TEST_CASE("weak,assignment") +{ + IStringable object = make(); + weak_ref ref1 = object; + + // Move constructor + weak_ref ref2 = std::move(ref1); + REQUIRE(ref1 == nullptr); + + // Copy constructor + weak_ref ref3 = ref2; + REQUIRE(ref2 == ref3); + + // Copy assignment + ref1 = ref2; + REQUIRE(ref1 == ref2); + REQUIRE(ref1 == ref3); + + // Move assignment + ref1 = std::move(ref2); + REQUIRE(ref2 == nullptr); + REQUIRE(ref1 == ref3); + + // Copy assignment from const + ref2 = static_cast const&>(ref1); + REQUIRE(ref1 == ref2); + + // Move assignment from const + ref1 = static_cast const&&>(ref2); + REQUIRE(ref1 == ref2); + + // Constructed from com_ref braced constructor + weak_ref yikes{ { nullptr, take_ownership_from_abi } }; + + // Not constructible from L"" (because Uri constructor is explicit) + static_assert(!std::is_constructible_v, const wchar_t*>); + + // Constructible from com_ptr because com_ptr is + // implicitly convertible to com_ptr. + struct Derived : WeakWithSelfReference {}; + weak_ref decay{ winrt::com_ptr{nullptr} }; +} + TEST_CASE("weak,module_lock") { uint32_t object_count = get_module_lock(); @@ -344,3 +406,10 @@ TEST_CASE("weak,no_module_lock") REQUIRE(get_module_lock() == object_count); } +TEST_CASE("weak,self") +{ + // The REQUIRE statements are in the WeakWithSelfReference class itself. + IStringable a = make(); + a.ToString(); + a = nullptr; +}