Data race on _pplx_g_sched_t::get_scheduler()

[whoan]

Google's thread sanitizer is reporting a data race on _pplx_g_sched_t::get_scheduler() function which I discovered creating multiple websocket instances at the same time on Linux.

You can run this simpler snippet to reproduce:

#include <thread>
#include <cpprest/ws_client.h>

int main() {
  //pplx::get_ambient_scheduler();  // uncomment this line to fix the data race
  std::thread a([] () { pplx::get_ambient_scheduler(); });
  std::thread b([] () { pplx::get_ambient_scheduler(); });
  a.join();
  b.join();
  return 0;
}

This is sanitizer's output:

==================
WARNING: ThreadSanitizer: data race (pid=4653)
  Read of size 8 at 0x7f96cf428c50 by thread T2:
    #0 std::__shared_ptr<pplx::scheduler_interface, (__gnu_cxx::_Lock_policy)2>::operator bool() const /usr/include/c++/8.2.1/bits/shared_ptr_base.h:1291 (libcpprest.so.2.10+0xa787a2)
    #1 pplx::_pplx_g_sched_t::get_scheduler() /home/bambino/projects/tradehelm/libfix-adapter/vendor/cpprest/Release/src/pplx/pplx.cpp:77 (libcpprest.so.2.10+0xa784e4)
    #2 pplx::get_ambient_scheduler() /home/bambino/projects/tradehelm/libfix-adapter/vendor/cpprest/Release/src/pplx/pplx.cpp:125 (libcpprest.so.2.10+0xa77e36)
    #3 operator() /data/projects/pruebas/c++/pplx-ambient-scheduler.cpp:7 (a.out+0x33a9)
    #4 __invoke_impl<void, main()::<lambda()> > /usr/include/c++/8.2.1/bits/invoke.h:60 (a.out+0x3b4e)
    #5 __invoke<main()::<lambda()> > /usr/include/c++/8.2.1/bits/invoke.h:95 (a.out+0x366b)
    #6 _M_invoke<0> /usr/include/c++/8.2.1/thread:244 (a.out+0x4198)
    #7 operator() /usr/include/c++/8.2.1/thread:253 (a.out+0x40c3)
    #8 _M_run /usr/include/c++/8.2.1/thread:196 (a.out+0x4024)
    #9 execute_native_thread_routine /build/gcc/src/gcc/libstdc++-v3/src/c++11/thread.cc:80 (libstdc++.so.6+0xbc062)

  Previous write of size 8 at 0x7f96cf428c50 by thread T1:
    #0 std::enable_if<std::__and_<std::__not_<std::__is_tuple_like<pplx::scheduler_interface*> >, std::is_move_constructible<pplx::scheduler_interface*>, std::is_move_assignable<pplx::scheduler_interface*> >::value, void>::type std::swap<pplx::scheduler_interface*>(pplx::scheduler_interface*&, pplx::scheduler_interface*&) /usr/include/c++/8.2.1/bits/move.h:195 (libcpprest.so.2.10+0xa78f48)
    #1 std::__shared_ptr<pplx::scheduler_interface, (__gnu_cxx::_Lock_policy)2>::swap(std::__shared_ptr<pplx::scheduler_interface, (__gnu_cxx::_Lock_policy)2>&) /usr/include/c++/8.2.1/bits/shared_ptr_base.h:1304 (libcpprest.so.2.10+0xa78da8)
    #2 std::enable_if<std::__sp_compatible_with<pplx::details::linux_scheduler*, pplx::scheduler_interface*>::value, std::__shared_ptr<pplx::scheduler_interface, (__gnu_cxx::_Lock_policy)2>&>::type std::__shared_ptr<pplx::scheduler_interface, (__gnu_cxx::_Lock_policy)2>::operator=<pplx::details::linux_scheduler>(std::__shared_ptr<pplx::details::linux_scheduler, (__gnu_cxx::_Lock_policy)2>&&) /usr/include/c++/8.2.1/bits/shared_ptr_base.h:1251 (libcpprest.so.2.10+0xa78bac)
    #3 std::enable_if<std::is_assignable<std::__shared_ptr<pplx::scheduler_interface, (__gnu_cxx::_Lock_policy)2>&, std::shared_ptr<pplx::details::linux_scheduler> >::value, std::shared_ptr<pplx::scheduler_interface>&>::type std::shared_ptr<pplx::scheduler_interface>::operator=<pplx::details::linux_scheduler>(std::shared_ptr<pplx::details::linux_scheduler>&&) /usr/include/c++/8.2.1/bits/shared_ptr.h:343 (libcpprest.so.2.10+0xa78947)
    #4 pplx::_pplx_g_sched_t::get_scheduler() /home/bambino/projects/tradehelm/libfix-adapter/vendor/cpprest/Release/src/pplx/pplx.cpp:82 (libcpprest.so.2.10+0xa7853c)
    #5 pplx::get_ambient_scheduler() /home/bambino/projects/tradehelm/libfix-adapter/vendor/cpprest/Release/src/pplx/pplx.cpp:125 (libcpprest.so.2.10+0xa77e36)
    #6 operator() /data/projects/pruebas/c++/pplx-ambient-scheduler.cpp:6 (a.out+0x3333)
    #7 __invoke_impl<void, main()::<lambda()> > /usr/include/c++/8.2.1/bits/invoke.h:60 (a.out+0x3910)
    #8 __invoke<main()::<lambda()> > /usr/include/c++/8.2.1/bits/invoke.h:95 (a.out+0x350d)
    #9 _M_invoke<0> /usr/include/c++/8.2.1/thread:244 (a.out+0x41f0)
    #10 operator() /usr/include/c++/8.2.1/thread:253 (a.out+0x412d)
    #11 _M_run /usr/include/c++/8.2.1/thread:196 (a.out+0x406e)
    #12 execute_native_thread_routine /build/gcc/src/gcc/libstdc++-v3/src/c++11/thread.cc:80 (libstdc++.so.6+0xbc062)

  Location is global 'pplx::_pplx_g_sched' of size 32 at 0x7f96cf428c40 (libcpprest.so.2.10+0x000000d58c50)

  Thread T2 (tid=4656, running) created by main thread at:
    #0 pthread_create /build/gcc/src/gcc/libsanitizer/tsan/tsan_interceptors.cc:915 (libtsan.so.0+0x2bf03)
    #1 __gthread_create /build/gcc/src/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/x86_64-pc-linux-gnu/bits/gthr-default.h:662 (libstdc++.so.6+0xbc359)
    #2 std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)()) /build/gcc/src/gcc/libstdc++-v3/src/c++11/thread.cc:135 (libstdc++.so.6+0xbc359)
    #3 main /data/projects/pruebas/c++/pplx-ambient-scheduler.cpp:7 (a.out+0x3435)

  Thread T1 (tid=4655, finished) created by main thread at:
    #0 pthread_create /build/gcc/src/gcc/libsanitizer/tsan/tsan_interceptors.cc:915 (libtsan.so.0+0x2bf03)
    #1 __gthread_create /build/gcc/src/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/x86_64-pc-linux-gnu/bits/gthr-default.h:662 (libstdc++.so.6+0xbc359)
    #2 std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)()) /build/gcc/src/gcc/libstdc++-v3/src/c++11/thread.cc:135 (libstdc++.so.6+0xbc359)
    #3 main /data/projects/pruebas/c++/pplx-ambient-scheduler.cpp:6 (a.out+0x3422)

SUMMARY: ThreadSanitizer: data race /usr/include/c++/8.2.1/bits/shared_ptr_base.h:1291 in std::__shared_ptr<pplx::scheduler_interface, (__gnu_cxx::_Lock_policy)2>::operator bool() const
==================

However, I don't see the actual data race on said function:

    sched_ptr get_scheduler()
    {
        switch (m_state)
        {
            case post_ctor:
                // This is the 99.9% case.

                if (!m_scheduler)
                {
                    ::pplx::details::_Scoped_spin_lock lock(m_spinlock);
                    if (!m_scheduler)
                    {
                        m_scheduler = std::make_shared<::pplx::default_scheduler_t>();
                    }
                }

                return m_scheduler;
            default:
                // This case means the global m_scheduler is not available.
                // We spin off an individual scheduler instead.
                return std::make_shared<::pplx::default_scheduler_t>();
        }
    }

It's easy to avoid the WARNING reported by the sanitizer, making a call to pplx::get_ambient_scheduler() beforehand. In my application, I use a global variable which makes a call to the function and I wonder if something similar should be applied to cpprestsdk to ease the task to other developers seeing the same data-race.

An alternative might be to call get_scheduler in _pplx_g_sched_t's constructor. Something like this:

_pplx_g_sched_t() { m_state = post_ctor; get_scheduler(); }

Thoughts? Ping @BillyONeal

[dimarusyy]

Sanitizer is correct as there is no atomic access to m_scheduler::operator bool() on if (!m_scheduler) line. Two approaches are possible: either creating object in ctor (but I guess lazy construction should be here by design) or making m_scheduler atomic.

[whoan]

I see @dimarusyy , but as the condition is checked again after the lock, I thought it wasn't considered a data race. From my point of view, m_scheduler not being atomic is harmless in this case, but I agree that something should be done to avoid the WARNING on Sanitizer.

[dimarusyy]

@BillyONeal , please review changes as they are rather critical in the scope of thread-safety.
Btw, I was considering using lock-free solution but rejected for now.

[BillyONeal]

Right, it can't be made an atomic because it's a shared_ptr.

A lot of this code predates atomics being in C++. Unfortuantely this is not the first case of casablanca code expecting shared_ptr to act like pointers for atomic access :(.

[dimarusyy]

I wonder if std::atomic_store()/std::atomic_load() may be used. I updated PR, but need to check what is wrong with cancel_with_error test on ubuntu-apt.

[whoan]

@BillyONeal I am still seeing a related data race in v2.10.12 (executing the same snippet I posted above):

WARNING: ThreadSanitizer: data race (pid=26528)
  Atomic write of size 4 at 0x7b0800001008 by thread T2:
    #0 __tsan_atomic32_fetch_add /build/gcc/src/gcc/libsanitizer/tsan/tsan_interface_atomic.cc:614 (libtsan.so.0+0x6968e)
    #1 __atomic_add /usr/include/c++/8.2.1/ext/atomicity.h:53 (a.out+0x12988)
    #2 __atomic_add_dispatch /usr/include/c++/8.2.1/ext/atomicity.h:96 (a.out+0x12aea)
    #3 std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_add_ref_copy() /usr/include/c++/8.2.1/bits/shared_ptr_base.h:139 (a.out+0x16d3c)
    #4 std::__shared_count<(__gnu_cxx::_Lock_policy)2>::operator=(std::__shared_count<(__gnu_cxx::_Lock_policy)2> const&) /usr/include/c++/8.2.1/bits/shared_ptr_base.h:725 (libcpprest.so.2.10+0x7eed22)
    #5 __invoke_impl<void, main()::<lambda()> > /usr/include/c++/8.2.1/bits/invoke.h:60 (a.out+0x11e04)
    #6 __invoke<main()::<lambda()> > /usr/include/c++/8.2.1/bits/invoke.h:95 (a.out+0x11921)
    #7 _M_invoke<0> /usr/include/c++/8.2.1/thread:244 (a.out+0x126c0)
    #8 operator() /usr/include/c++/8.2.1/thread:253 (a.out+0x125eb)
    #9 _M_run /usr/include/c++/8.2.1/thread:196 (a.out+0x1254c)
    #10 execute_native_thread_routine /build/gcc/src/gcc/libstdc++-v3/src/c++11/thread.cc:80 (libstdc++.so.6+0xbc062)

  Previous write of size 8 at 0x7b0800001008 by thread T1:
    #0 operator new(unsigned long) /build/gcc/src/gcc/libsanitizer/tsan/tsan_new_delete.cc:42 (libtsan.so.0+0x75d0c)
    #1 __gnu_cxx::new_allocator<std::_Sp_counted_ptr_inplace<pplx::details::linux_scheduler, std::allocator<pplx::details::linux_scheduler>, (__gnu_cxx::_Lock_policy)2> >::allocate(unsigned long, void const*) /usr/include/c++/8.2.1/ext/new_allocator.h:111 (libcpprest.so.2.10+0x9bd3aa)
    #2 __invoke_impl<void, main()::<lambda()> > /usr/include/c++/8.2.1/bits/invoke.h:60 (a.out+0x11bc6)
    #3 __invoke<main()::<lambda()> > /usr/include/c++/8.2.1/bits/invoke.h:95 (a.out+0x117c3)
    #4 _M_invoke<0> /usr/include/c++/8.2.1/thread:244 (a.out+0x12718)
    #5 operator() /usr/include/c++/8.2.1/thread:253 (a.out+0x12655)
    #6 _M_run /usr/include/c++/8.2.1/thread:196 (a.out+0x12596)
    #7 execute_native_thread_routine /build/gcc/src/gcc/libstdc++-v3/src/c++11/thread.cc:80 (libstdc++.so.6+0xbc062)

  Location is heap block of size 24 at 0x7b0800001000 allocated by thread T1:
    #0 operator new(unsigned long) /build/gcc/src/gcc/libsanitizer/tsan/tsan_new_delete.cc:42 (libtsan.so.0+0x75d0c)
    #1 __gnu_cxx::new_allocator<std::_Sp_counted_ptr_inplace<pplx::details::linux_scheduler, std::allocator<pplx::details::linux_scheduler>, (__gnu_cxx::_Lock_policy)2> >::allocate(unsigned long, void const*) /usr/include/c++/8.2.1/ext/new_allocator.h:111 (libcpprest.so.2.10+0x9bd3aa)
    #2 __invoke_impl<void, main()::<lambda()> > /usr/include/c++/8.2.1/bits/invoke.h:60 (a.out+0x11bc6)
    #3 __invoke<main()::<lambda()> > /usr/include/c++/8.2.1/bits/invoke.h:95 (a.out+0x117c3)
    #4 _M_invoke<0> /usr/include/c++/8.2.1/thread:244 (a.out+0x12718)
    #5 operator() /usr/include/c++/8.2.1/thread:253 (a.out+0x12655)
    #6 _M_run /usr/include/c++/8.2.1/thread:196 (a.out+0x12596)
    #7 execute_native_thread_routine /build/gcc/src/gcc/libstdc++-v3/src/c++11/thread.cc:80 (libstdc++.so.6+0xbc062)

  Thread T2 (tid=26532, running) created by main thread at:
    #0 pthread_create /build/gcc/src/gcc/libsanitizer/tsan/tsan_interceptors.cc:915 (libtsan.so.0+0x2bf03)
    #1 __gthread_create /build/gcc/src/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/x86_64-pc-linux-gnu/bits/gthr-default.h:662 (libstdc++.so.6+0xbc359)
    #2 std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)()) /build/gcc/src/gcc/libstdc++-v3/src/c++11/thread.cc:135 (libstdc++.so.6+0xbc359)
    #3 main /data/projects/pruebas/c++/pplx-ambient-scheduler.cpp:7 (a.out+0x116eb)

  Thread T1 (tid=26531, finished) created by main thread at:
    #0 pthread_create /build/gcc/src/gcc/libsanitizer/tsan/tsan_interceptors.cc:915 (libtsan.so.0+0x2bf03)
    #1 __gthread_create /build/gcc/src/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/x86_64-pc-linux-gnu/bits/gthr-default.h:662 (libstdc++.so.6+0xbc359)
    #2 std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)()) /build/gcc/src/gcc/libstdc++-v3/src/c++11/thread.cc:135 (libstdc++.so.6+0xbc359)
    #3 main /data/projects/pruebas/c++/pplx-ambient-scheduler.cpp:6 (a.out+0x116d8)

SUMMARY: ThreadSanitizer: data race /usr/include/c++/8.2.1/ext/atomicity.h:53 in __atomic_add
==================
ThreadSanitizer: reported 1 warnings

I will stick with my workaround (making a call to pplx::get_ambient_scheduler() beforehand) as that eliminates the data race-

[dimarusyy]

Looks like lock is missing in default case of switch statement.
@BillyONeal, do you prefer to have m_state and fix with adding lock to default: case or update with acquire/release synchronization via m_state?

[garethsb]

I think the constructor of _Spin_lock shouldn't have a member initializer for _M_lock member, at the very least not with 0. If I understand https://en.cppreference.com/w/cpp/atomic/ATOMIC_FLAG_INIT correctly, the right thing is probably a default member initializer at the point of declaration:

    std::atomic_flag _M_lock = ATOMIC_FLAG_INIT;

[BillyONeal]

@dimarusyy There's no lock there because there's no mutation of any shared variable in that location.

@garethsb-sony ATOMIC_FLAG_INIT just sets it to 0 on every implementation of which I am aware; that thing exists for a C compatibility scenario that never really worked out.

[garethsb]

Yes, but using 0 in a constructor call fails to compile on some implementations...

[garethsb]

Specifically, VS 2015 Update 3:

error C2664: 'std::atomic_flag::atomic_flag(const std::atomic_flag &)':
 cannot convert argument 1 from 'int' to 'const std::atomic_flag &'

[BillyONeal]

@garethsb-sony True; this file does not get compiled on Windows at all (which is why it passes CI https://dev.azure.com/vclibs/cpprestsdk/_build/results?buildId=359 ). We should probably just value init the thing. That still doesn't explain why asan is still unhappy though.

[garethsb]

It does get compiled on Windows with CPPREST_FORCE_PPLX...