Skip to content
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.

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

CCB: Changed logic of work using boost conditional variable #3180

Closed
LitvinenkoIra opened this issue Dec 6, 2019 · 4 comments
Closed

CCB: Changed logic of work using boost conditional variable #3180

LitvinenkoIra opened this issue Dec 6, 2019 · 4 comments

Comments

@LitvinenkoIra
Copy link
Contributor

Bug Report

Data races related to Logger

Detailed analysis:

Note that the same mutex is locked before the shared data is updated,
but that mutex does not have to be locked across the call to notify_one.:
https://www.boost.org/doc/libs/1_66_0/doc/html/thread/synchronization.html#thread.synchronization.condvar_ref

In function Thread::threadFunc

thread->run_lock_.Acquire();
sync_primitives::AutoLock auto_lock(thread->state_lock_);
thread->run_lock_.Release();

This is not a lock for acquiring another lock.
This code part ensures correct notification sequence between Thread::start and threadFunc.
In Thread::start function after thread is created, we call Wait(),
but we cannot guarantee order of precedence of the following calls :
"Wait() in Thread::start" or "Broadcast in threadFunc".
sync_primitives::AutoLock auto_lock(thread->run_lock_); - is used to
guarantee call to "Wait() in Thread::start" before Broadcast.

Reproduction Steps
  1. TBD
Expected Behavior

SDL should not crash

Observed Behavior

SDL crashed

OS & Version Information

@SNiukalov
Copy link
Contributor

After merge #3471 we do not use multiple conditional variable or lockable objects for guarantee order of precedence of the following calls :
"Wait() in Thread::Start, Stop and Join" or "Broadcast in threadFunc".

In the current implementation, we use a single instance for a conditional variable and a lockable object.

After merge #3471 all regression tests are passed.
So I believe it is redundant for the current implementation.

@LitvinenkoIra
Copy link
Contributor Author

@theresalech we believe that this issue already fixed by #3471

@theresalech
Copy link
Contributor

@LitvinenkoIra thank you for letting us know! We will review as time allows before the Core 7.0 release.

@iCollin
Copy link
Collaborator

iCollin commented Sep 30, 2020

Closed via #3471

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

No branches or pull requests

4 participants