CppCoreGuidelines
CppCoreGuidelines copied to clipboard
isocpp
[Proposal] Coroutine class functions on refcounted classes should keep the class instance alive until completion (CP.coro section)
Coroutine class functions on refcounted classes should keep the class instance alive until completion
Reason
When a coroutine class method reaches the first suspension point, such as co_await, the synchronous portion of the coroutine returns. After that point there is nothing in the coroutine keeping the reference count from going to zero and destructing the instance pointed to by the the this pointer. This can be avoided by ensuring that a reference count is held in a local variable that will live until the coroutine completes.
In some cases all access to class fields happen before the first suspension point, so these accesses are safe. However, it is fragile to rely on that because subsequent changes to the function may add or move suspension points which would reintroduce this class of bug. It is safer to always ensure a reference count is held for the lifetime of the coroutine.
Example, Bad
class Class: public std::enable_shared_from_this<Class>
{
std::future<int> do_something()
{
co_await something();
return m_classField; // DANGER: the this pointer may have been freed while the coroutine was suspended
}
private:
int m_classField{};
};
Example, Good
class Class: public std::enable_shared_from_this<Class>
{
std::future<int> do_something()
{
const auto strong_this = shared_from_this();
co_await something();
return m_classField; // OK: the this pointer is kept alive by strong_this
}
private:
int m_classField{};
};
Could the bad example be something deliberate? I haven't practical experience with coroutines, but wonder if there are cases, where you might want to cancel the coroutine preliminary when no one else uses the object anymore.
I think that would apply only to cases where the dev knows what they're doing[TM]. Without keeping the reference alive, you may well run into situations where an innocently looking function call is actually a method call on this. Which, again, results in the problems mentioned in the original post. Honestly, this all makes me think whether one shouldn't just discourage coroutine methods alltogether.
knows what they're doing[TM].
Absolutely - I'm thinking about something that would be e.g. encapsulated inside the very class that this coroutine would be a (potentially private) member. I'm definetly not talking about a public library interfaces.
I tried to cover that suggestion with this portion:
In some cases all access to class fields happen before the first suspension point, so these accesses are safe. However, it is fragile to rely on that because subsequent changes to the function may add or move suspension points which would reintroduce this class of bug.
As long as class fields are only accessed before the first co_await, or are not accessed at all in the method, it is technically safe. Even if this destructs during an await it won't matter because it isn't needed beyond that point. I would argue that the avoided reference count add/subtract are not worth the very real risk of introducing bugs with innocent-looking changes in the future.
As for cancelation, one approach that seems to work well enough is to use an exception to stop execution. That is what cppwinrt does via winrt::hresult_canceled.
I think I explained myself poorly. My point question was if there are patterns, where you a) know the parent object will live as long as the couritine is running and b) you want to be able to destroy the enclosing object even before the coroutine is completed (but e.g. suspended) and you cancel the coroutine in the destructor.
Again, I can't imagine that something like that would be good design as part of a public interface because - as you pointed out - it is too fragile, but I wonder if something like that exists as a implementation technique.
That seems fraught with peril. If you want the outer object to be able to destruct then it is probably best to have the coroutine not be a class method. Then it will have to take everything it needs as a parameter and the lifetime safety is handled. Cancelling from the class destructor could still be a possibility if that is desirable.
Editors call: Is this basically the same issue as the one about banning reference parameters, and perhaps the two could be handled together/merged?
also, doesn't the proposed fix fail if the coroutine is started lazily, as in, when promise.initial_suspend() returns suspend_always?
@hsutter this is distinct from the one about reference parameters because the concern is with the this pointer. If the lifetime of the current class instance is not ensured then it can be freed while the coroutine is suspended. This is true even for a method with no parameters.
@cubbimew that is an interesting question. I do not have first-hand experience with lazy coroutines so I'm not completely sure, but it sounds like that could pose a problem. A shared_ptr parameter holding the this pointer would probably be necessary to ensure safety in that scenario. Or have the lazy coroutine be a non-class method.