Skip to content

Conversation

@oldnewthing
Copy link
Member

@oldnewthing oldnewthing commented Jan 29, 2022

root_implements::NonDelegatingRelease sets m_references = 1 prior to initiating destruction so that the destructor sees a nonzero reference count. As a side effect, it also breaks the connection to the weak reference control block.

~root_implements performs a bonus subtract_reference to clean up any weak reference control block that may have re-materialized during destruction. The extra decrement is safe because we set m_references = 1 earlier.

However, if the root_implements::Release was overriden by a derived class, the derived class's Release may not do the same m_references = 1 trick, and then the bonus decrement in the destructor will decrement below zero.

Who would override root_implements::Release? promise_base, that's who. As a result, a weak reference to a coroutine turns into poison when the coroutine destructs, because the control block now has a strong reference of 4 billion and a pointer to an already-destroyed object.

The logic to set m_references to 1 is now moved into subtract_reference so that derived classes don't have to know this One Weird Trick. The actual decrementing happens in subtract_final_reference, which is the version that the root_implements destructor calls, since it knows that the object is finally going away and isn't preparing for destruction.

Setting m_references to 1 had been done with memory_order_cst (default), but I think memory_order_relaxed is sufficient. Since the external reference count is zero, there are no other threads with access to the object (outstanding weak references will not resolve), so there are no data dependencies.

Created new tests to validate the "weak reference created during destruction" code path, as well as to validate that weak references to coroutines work. (The weak,coroutine test failed prior to the fix.)

`root_implements::NoDelegatingRelease` sets `m_references = 1` prior to
initiating destruction so that the destructor sees a nonzero
reference count. As a side effect, it also breaks the connection
to the weak reference control block.

`~root_implements` performs a bonus `subtract_reference` to clean
up any weak reference control block that may have re-materialized
during destruction. The extra decrement is safe because we set
`m_references = 1` earlier.

However, if the `root_implements::Release` was overriden by a
base class, the base class's `Release` may not do the same
`m_references = 1` trick, and then the bonus decrement in the
destructor will decrement below zero.

Who would override `root_implements::Release`? `promise_base`,
that's who. As a result, a weak reference to a coroutine turns
into poison when the coroutine destructs, because the control
block now has a strong reference of 4 billion and a pointer to
an already-destroyed object.

The logic to set `m_references` to 1 is now moved into
`subtract_reference` so that derived classes don't have
to know this One Weird Trick. The actual decrementing happens
in `subtract_final_reference`, which is the version that
the `root_implements` destructor calls, since it knows that the
object is finally going away and isn't preparing for destruction.
@oldnewthing oldnewthing requested a review from kennykerr January 29, 2022 19:19
Copy link
Collaborator

@kennykerr kennykerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Also, just silently skip the coroutine tests if coroutines are
not enabled. Don't need to be chatty about it.
@kennykerr kennykerr merged commit 24650de into microsoft:master Jan 31, 2022
@oldnewthing oldnewthing deleted the weak-coroutine branch September 5, 2023 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants