Data race on _pplx_g_sched_t::get_scheduler() (#1085) #1087
Conversation
dimarusyy
commented
Mar 26, 2019
dimarusyy
commented
- Make singleton thread-safe
|
|
- use C++11 compatible implementation
| } | ||
|
|
||
| return m_scheduler; |
| { | ||
| switch (m_state) | ||
| { | ||
| sched_ptr sptr = std::atomic_load_explicit(&m_scheduler, std::memory_order_consume); |
There was a problem hiding this comment.
Not sure how I feel about memory_order_consume; even SG1 thinks that thing is effectively broken. Any objections to changing this to acquire?
There was a problem hiding this comment.
consume and acquire should be equivalent here as atomic is not used for synchronization. Anyway, I'll update with acquire.
| // 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Am I right that you suggest to drop double-lock here and just acquire spinlock on every call to get_scheduler()?
There was a problem hiding this comment.
@dimarusyy Yes, that's what my alternative does. Does that solution look acceptable to you?
There was a problem hiding this comment.
I'm fine as you accepted spinlock acquiring on every call to get_scheduler(). Thanks for comment and update.
|
Can you look at this alternative and let me know if that makes you happy? |
|
I mean this alternative: #1088 |
