Skip to content

Data race on _pplx_g_sched_t::get_scheduler() (#1085) #1087

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Data race on _pplx_g_sched_t::get_scheduler() (#1085) #1087

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
545aac3
#1085 : Data race on _pplx_g_sched_t::get_scheduler()
dimarusyy Mar 26, 2019
f1a1772
#1085 : Data race on _pplx_g_sched_t::get_scheduler()
dimarusyy Mar 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 16 additions & 39 deletions Release/src/pplx/pplx.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

#if !defined(_WIN32) || CPPREST_FORCE_PPLX

#include <atomic>
#include "pplx/pplx.h"

// Disable false alarm code analyze warning
Expand Down Expand Up @@ -63,58 +64,34 @@ static struct _pplx_g_sched_t
{
typedef std::shared_ptr<pplx::scheduler_interface> sched_ptr;

_pplx_g_sched_t() { m_state = post_ctor; }

~_pplx_g_sched_t() { m_state = post_dtor; }

sched_ptr get_scheduler()
{
switch (m_state)
{
sched_ptr sptr = std::atomic_load_explicit(&m_scheduler, std::memory_order_consume);
Copy link
Member

Choose a reason for hiding this comment

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

Not sure how I feel about memory_order_consume; even SG1 thinks that thing is effectively broken. Any objections to changing this to acquire?

Copy link
Contributor Author

@dimarusyy dimarusyy Mar 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

consume and acquire should be equivalent here as atomic is not used for synchronization. Anyway, I'll update with acquire.

if (!sptr)
{
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>();
::pplx::details::_Scoped_spin_lock lock(m_spinlock);
Copy link
Member

Choose a reason for hiding this comment

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

As far as I am aware, all the major shared_ptr implementations actually use spinlocks to implement this load operation anyway; perhaps we should just drop the check and take the spinlock every time. It does limit scalability of the system but this shared_ptr city wasn't going to win awards for that anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Am I right that you suggest to drop double-lock here and just acquire spinlock on every call to get_scheduler()?

Copy link
Member

Choose a reason for hiding this comment

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

@dimarusyy Yes, that's what my alternative does. Does that solution look acceptable to you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fine as you accepted spinlock acquiring on every call to get_scheduler(). Thanks for comment and update.

sptr = std::atomic_load_explicit(&m_scheduler, std::memory_order_relaxed);
if (!sptr)
{
sptr = std::make_shared<::pplx::default_scheduler_t>();
std::atomic_store_explicit(&m_scheduler, sptr, std::memory_order_release);
}
}

return m_scheduler;
Copy link
Member

Choose a reason for hiding this comment

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

Bug, needs to return sptr.

}

void set_scheduler(sched_ptr scheduler)
{
if (m_state == pre_ctor || m_state == post_dtor)
{
throw invalid_operation("Scheduler cannot be initialized now");
}

::pplx::details::_Scoped_spin_lock lock(m_spinlock);

if (m_scheduler != nullptr)
if (std::atomic_load_explicit(&m_scheduler, std::memory_order_consume) != nullptr)
{
throw invalid_operation("Scheduler is already initialized");
}

m_scheduler = std::move(scheduler);
::pplx::details::_Scoped_spin_lock lock(m_spinlock);
std::atomic_store_explicit(&m_scheduler, scheduler, std::memory_order_release);
}

enum
{
pre_ctor = 0,
post_ctor = 1,
post_dtor = 2
} m_state;

private:
pplx::details::_Spin_lock m_spinlock;
sched_ptr m_scheduler;
Expand Down