From 5a65ab7b5d78b1684d8c435d3bc0e94e8946abd2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Hugo=20Beauz=C3=A9e-Luyssen?= <hugo@beauzee.fr>
Date: Fri, 11 Feb 2022 11:44:54 +0100
Subject: [PATCH 1/2] parser: Don't toggle idle state outside of the worker

Changing the idle state is likely to cause a deadlock on startup when
tasks are restored.
parser::restore() is called from MediaLibrary::initialize() with the
medialib lock held, and changing the idle state synchronously from that
thread would cause onIdleChange to be invoked, which will try to lock
the mutex recursively, leading to a deadlock.
---
 src/parser/ParserWorker.cpp | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/src/parser/ParserWorker.cpp b/src/parser/ParserWorker.cpp
index 3daba8da..027485cf 100644
--- a/src/parser/ParserWorker.cpp
+++ b/src/parser/ParserWorker.cpp
@@ -93,19 +93,6 @@ void Worker::parse( std::shared_ptr<Task> t )
     {
         std::lock_guard<compat::Mutex> lock( m_lock );
 
-        // Avoid flickering from idle/not idle when not many tasks are running.
-        // The thread calling parse for the next parser step might not have
-        // something left to do and would turn idle, potentially causing all
-        // services to be idle for a very short time, until this parser
-        // thread awakes/starts, causing the global parser idle state to be
-        // restored back to false.
-        // Since we are queuing a task, we already know that this thread is
-        // not idle
-        // However, we must not override the idle state if the worker was paused
-        // since the pausing thread is waiting for the worker to go idle.
-        if ( m_paused == false )
-            setIdle( false );
-
         m_tasks.push( std::move( t ) );
         if ( m_thread.get_id() == compat::Thread::id{} )
         {
@@ -121,9 +108,6 @@ void Worker::parse( std::vector<std::shared_ptr<Task>> tasks )
     {
         std::lock_guard<compat::Mutex> lock( m_lock );
 
-        if ( m_paused == false )
-            setIdle( false );
-
         for ( auto& t : tasks )
             m_tasks.push( std::move( t ) );
         if ( m_thread.get_id() == compat::Thread::id{} )
@@ -168,7 +152,7 @@ void Worker::mainloop()
     // that the underlying service has been deleted already.
     std::string serviceName = m_service->name();
     LOG_INFO("Entering ParserService [", serviceName, "] thread");
-    m_parserCb->onIdleChanged( false );
+    setIdle( false );
 
     // Run the service specific initializer
     m_service->initialize( m_ml );
-- 
GitLab


From 20e0c695561759a77929ac7281a1bf72bb95febc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Hugo=20Beauz=C3=A9e-Luyssen?= <hugo@beauzee.fr>
Date: Fri, 11 Feb 2022 11:59:26 +0100
Subject: [PATCH 2/2] parser: Use explicit memory ordering for idle state

---
 src/parser/ParserWorker.cpp | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/parser/ParserWorker.cpp b/src/parser/ParserWorker.cpp
index 027485cf..bef52039 100644
--- a/src/parser/ParserWorker.cpp
+++ b/src/parser/ParserWorker.cpp
@@ -129,7 +129,7 @@ void Worker::initialize( MediaLibrary* ml, IParserCb* parserCb,
 
 bool Worker::isIdle() const
 {
-    return m_idle;
+    return m_idle.load( std::memory_order_acquire );
 }
 
 void Worker::flush()
@@ -251,9 +251,9 @@ void Worker::setIdle(bool isIdle)
 {
     // Calling the idleChanged callback will trigger a call to isIdle, so set the value before
     // invoking it, otherwise we have an incoherent state.
-    if ( m_idle != isIdle )
+    auto expected = !isIdle;
+    if ( m_idle.compare_exchange_strong( expected, isIdle, std::memory_order_acq_rel ) == true )
     {
-        m_idle = isIdle;
         m_parserCb->onIdleChanged( isIdle );
     }
 }
-- 
GitLab