Skip to content
Snippets Groups Projects
Commit ea8a3b3b authored by Olivier Dion's avatar Olivier Dion :duck: Committed by Sébastien Blin
Browse files

scheduled_executor: Fix deadlock

If the scheduler's has a job that owns the last reference to the scheduler
itself, then the thread will try to join itself in the destructor of the
scheduler, resulting in `std::terminate()`.  Thus, detach the thread if the
scheduler is freed from its thread, because nobody is supposed to have a
reference to the thread.

This introduces a read after free of `running_`.  By making it a shared value,
the thread can keep a reference after destruction of the scheduler.

Gitlab: #545
Change-Id: I0118c99a5718d302f916099e24606add444546d8
parent f42ef06b
No related branches found
No related tags found
No related merge requests found
...@@ -23,8 +23,12 @@ ...@@ -23,8 +23,12 @@
namespace jami { namespace jami {
ScheduledExecutor::ScheduledExecutor() ScheduledExecutor::ScheduledExecutor()
: thread_([this] { : running_(std::make_shared<std::atomic<bool>>(true))
while (running_.load()) , thread_([this, is_running = running_] {
// The thread needs its own reference of `running_` in case the
// scheduler is destroyed within the thread because of a job
while (*is_running)
loop(); loop();
}) })
{} {}
...@@ -32,16 +36,25 @@ ScheduledExecutor::ScheduledExecutor() ...@@ -32,16 +36,25 @@ ScheduledExecutor::ScheduledExecutor()
ScheduledExecutor::~ScheduledExecutor() ScheduledExecutor::~ScheduledExecutor()
{ {
stop(); stop();
if (thread_.joinable())
if (not thread_.joinable()) {
return;
}
// Avoid deadlock
if (std::this_thread::get_id() == thread_.get_id()) {
thread_.detach();
} else {
thread_.join(); thread_.join();
} }
}
void void
ScheduledExecutor::stop() ScheduledExecutor::stop()
{ {
{ {
std::lock_guard<std::mutex> lock(jobLock_); std::lock_guard<std::mutex> lock(jobLock_);
running_ = false; *running_ = false;
jobs_.clear(); jobs_.clear();
} }
cv_.notify_all(); cv_.notify_all();
...@@ -106,7 +119,7 @@ ScheduledExecutor::loop() ...@@ -106,7 +119,7 @@ ScheduledExecutor::loop()
std::vector<Job> jobs; std::vector<Job> jobs;
{ {
std::unique_lock<std::mutex> lock(jobLock_); std::unique_lock<std::mutex> lock(jobLock_);
while (running_ and (jobs_.empty() or jobs_.begin()->first > clock::now())) { while (*running_ and (jobs_.empty() or jobs_.begin()->first > clock::now())) {
if (jobs_.empty()) if (jobs_.empty())
cv_.wait(lock); cv_.wait(lock);
else { else {
...@@ -114,7 +127,7 @@ ScheduledExecutor::loop() ...@@ -114,7 +127,7 @@ ScheduledExecutor::loop()
cv_.wait_until(lock, nextJob); cv_.wait_until(lock, nextJob);
} }
} }
if (not running_) if (not *running_)
return; return;
jobs = std::move(jobs_.begin()->second); jobs = std::move(jobs_.begin()->second);
jobs_.erase(jobs_.begin()); jobs_.erase(jobs_.begin());
......
...@@ -137,7 +137,7 @@ private: ...@@ -137,7 +137,7 @@ private:
void schedule(std::shared_ptr<Task>, time_point t); void schedule(std::shared_ptr<Task>, time_point t);
void reschedule(std::shared_ptr<RepeatedTask>, time_point t, duration dt); void reschedule(std::shared_ptr<RepeatedTask>, time_point t, duration dt);
std::atomic_bool running_ {true}; std::shared_ptr<std::atomic<bool>> running_;
std::map<time_point, std::vector<Job>> jobs_ {}; std::map<time_point, std::vector<Job>> jobs_ {};
std::mutex jobLock_ {}; std::mutex jobLock_ {};
std::condition_variable cv_ {}; std::condition_variable cv_ {};
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment