`std::atomic<std::shared_ptr>::wait` does not seem to care about control block difference. Is this a bug?

[zwhfly]

Discussed in #3555

^{Originally posted by zwhfly March 10, 2023}
This code seems to block forever under MSVC, but returns successfully under gcc/clanglibstdc++.

#include <memory>
#include <atomic>


int main()
{
    auto sp1 = std::make_shared<char>();
    auto holder = [sp1] {};
    auto sp2 = std::make_shared<decltype(holder)>(holder);
    std::shared_ptr<char> sp3{ sp2, sp1.get() };

    std::atomic<std::shared_ptr<char>> asp{ sp1 };
    asp.wait(sp3);

    return 0;
}

It is said here that

(atomic<shared_ptr>::wait) compares load(order) with old and if they are equivalent then blocks ...

Notes: two shared_ptrs are equivalent if they store the same pointer and either share ownership or are both empty.

I thought that sp1 and sp3 do not have the same control block, so they do not share ownership. Right? So the wait call should return immediately.

But by checking the source code, it seems that the STL implementation for std::atomic<std::shared_ptr>::wait compares the object pointer (the char *) only. It does not compare the control block pointer at all.

Is this a bug in STL? Or what am I missing here?

[StephanTLavavej]

It appears that this cannot be truly fixed in our current implementation, since it would involve expecting WaitOnAddress to wake up for a notification when the waited-on location (the object pointer) hasn't actually changed, but an unrelated location (the control block pointer) has changed. As https://devblogs.microsoft.com/oldnewthing/20201214-00/?p=104544 explains (thanks to Fulgen on Discord for finding this):

The customer was relying on an implementation detail: Waking by address wakes a waiter, even if the waiter’s wake criteria aren’t satisfied. This is legal behavior because Wait­On­Address is permitted to wake spuriously. The expectation is that upon waking, you go back and check your wake condition to confirm that it was a valid wake. (In this case, we check for a valid wake by inspecting the work queue.) The problem is that the customer was relying on the spurious wake.

Furthermore, @AlexGuteniev observed that even if we could rely on waking up when the wake criterion isn't satisfied, there's a timing issue. With #3655's proposed change:

    void _Wait(remove_extent_t<_Ty>* _Old_ptr, _Ref_count_base* const _Old_rep, memory_order) const noexcept {
        for (;;) {
            auto _Rep   = _Repptr._Lock_and_load();
            bool _Equal = _Ptr.load(memory_order_relaxed) == _Old_ptr && _Rep == _Old_rep;
            _Repptr._Store_and_unlock(_Rep);
            if (!_Equal) {
                break;
            }
// *** What if the control block (_Repptr) is changed here, and a notification is delivered before we start waiting on _Ptr? ***
            __std_atomic_wait_direct(&_Ptr, &_Old_ptr, sizeof(_Old_ptr), __std_atomic_wait_no_timeout);
        }
    }

It seems to me that we're trying to implement a condition variable with insufficient technology (we have to care about two locations, but we can only observe one). Fixing this appears to require an ABI break / representation change.