-
Notifications
You must be signed in to change notification settings - Fork 263
Allow an object to hold a weak reference to itself in a member #1038
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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<T>` 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<T>` 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<T>` in the body. There are several wrinkles to this.
1. The universal reference template type parameter has a default
type of `com_ref<T> const&`. This permits the passing of an anonymous
braced parameter list to construct a `com_ref<T>`.
2. We must use `std::is_convertible` to detect whether the `U&&`
can be converted to a `com_ref<T> const&`, rather than writing
`com_ref<T> 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<weak_ref<T>, U>` returns
the correct value. (If we did it as a `static_assert`, then
`std::is_constructible<weak_ref<T>, 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<T>`
into a helper function `from_com_ref`, so that the conversion
from `U&&` to `com_ref<T> 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<T>`, 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<HWIDGETNOTIFICATION,
decltype(&::UnregisterWidgetCallback),
::UnregisterWidgetCallback>;
struct MyClass : implements<MyClass, IInspectable>
{
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<MyClass>`, 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<MyClass, IInspectable>
{
// Order of declaration is important here.
// weak_self must be declared first so it destructs last.
weak_ref<MyClass> 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<default_interface<MyClass>> weak_self{ *this };
```
and
```cpp
auto strongThis = ((MyClass*)context)->weak_self.get();
if (strongThis) get_self<MyClass>(strongThis)->OnWidgetCallback();
```
Member
Author
|
Note that the dummy template parameter trick which we used to allow construction of |
kennykerr
approved these changes
Oct 11, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Full example is given below.
Prior to this change, you could not have a weak reference to yourself because the
winrt::weak_ref<T>required thatTbe 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<T>out of the function prototypes and into the bodies. By the time the bodies are expanded, the typeTwill be complete, and everything will work.This is easy to do with
get()since we can just make the function returnauto. 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 desiredcom_ref<T>in the body. There are several wrinkles to this.The universal reference template type parameter has a default type of
com_ref<T> const&. This permits the passing of an anonymous braced parameter list to construct acom_ref<T>, which was supported in the previous version.We must use
std::is_convertibleto detect whether theU&&can be converted to acom_ref<T> const&, rather than writingcom_ref<T> const& ref = object;because the latter is an explicit conversion, but parameter passing uses only implicit conversions.The
std::is_convertibletest must be performed as part of SFINAE, so thatstd::is_constructible<weak_ref<T>, U>returns the correct value. (If we did it as astatic_assertin the body, thenstd::is_constructible<weak_ref<T>, U>would always be true, even though not all choices forUsuccessfully compile.)To avoid code size explosion, we factor the
com_ref<T>into a helper functionfrom_com_ref, so that the conversion fromU&&tocom_ref<T> const&happens outside the helper function, and the helper function itself can be shared. Again, we cannot saycom_ref<T>in the prototype, so we make it a dependent type (that is in practice alwayscom_ref<T>).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_refis not implicitly convertible tocom_ref<T>, so theis_convertibleSFINAE covers this already.Added tests to validate behavior, particularly that we didn't mess up the default copy/move constructors, copy/move assignments, and implicit conversions.
Example
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 aMyClassthat 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 toget_strong()succeeds, but the thing you get back will not prevent destruction.)The solution is to use a
weak_ref<MyClass>, 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.If destruction of
MyClasshas begun, theweak_self.get()will fail, ands_OnWidgetCallbackwill just return without doing anything.This solution requires that it be possible to create a
weak_refto 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.
weak_ref<default_interface<MyClass>> weak_self{ *this };and