Add more thread safety to background jobs

Fixes https://gitlab.com/kicad/code/kicad/-/issues/15395

common/background_jobs_monitor.cpp

@@ -40,7 +40,7 @@
 class BACKGROUND_JOB_PANEL : public wxPanel
 {
 public:
-    BACKGROUND_JOB_PANEL( wxWindow* aParent, BACKGROUND_JOB* aJob ) :
+    BACKGROUND_JOB_PANEL( wxWindow* aParent, std::shared_ptr<BACKGROUND_JOB> aJob ) :
             wxPanel( aParent, wxID_ANY, wxDefaultPosition, wxSize( -1, 75 ),
                      wxBORDER_SIMPLE ),
             m_job( aJob )
@@ -86,7 +86,7 @@ private:
     wxGauge* m_progress;
     wxStaticText* m_stName;
     wxStaticText* m_stStatus;
-    BACKGROUND_JOB* m_job;
+    std::shared_ptr<BACKGROUND_JOB> m_job;
 };
 
 
@@ -127,7 +127,7 @@ public:
     }
 
 
-    void Add( BACKGROUND_JOB* aJob )
+    void Add( std::shared_ptr<BACKGROUND_JOB> aJob )
     {
         BACKGROUND_JOB_PANEL* panel = new BACKGROUND_JOB_PANEL( m_scrolledWindow, aJob );
         m_contentSizer->Add( panel, 0, wxEXPAND | wxALL, 2 );
@@ -141,7 +141,7 @@ public:
     }
 
 
-    void Remove( BACKGROUND_JOB* aJob )
+    void Remove( std::shared_ptr<BACKGROUND_JOB> aJob )
     {
         auto it = m_jobPanels.find( aJob );
         if( it != m_jobPanels.end() )
@@ -154,7 +154,7 @@ public:
         }
     }
 
-    void UpdateJob( BACKGROUND_JOB* aJob )
+    void UpdateJob( std::shared_ptr<BACKGROUND_JOB> aJob )
     {
         auto it = m_jobPanels.find( aJob );
         if( it != m_jobPanels.end() )
@@ -167,12 +167,12 @@ public:
 private:
     wxScrolledWindow* m_scrolledWindow;
     wxBoxSizer*       m_contentSizer;
-    std::unordered_map<BACKGROUND_JOB*, BACKGROUND_JOB_PANEL*> m_jobPanels;
+    std::unordered_map<std::shared_ptr<BACKGROUND_JOB>, BACKGROUND_JOB_PANEL*> m_jobPanels;
 };
 
 
 BACKGROUND_JOB_REPORTER::BACKGROUND_JOB_REPORTER( BACKGROUND_JOBS_MONITOR* aMonitor,
-                                                  BACKGROUND_JOB* aJob ) :
+                                                  std::shared_ptr<BACKGROUND_JOB> aJob ) :
         PROGRESS_REPORTER_BASE( 1 ),
         m_monitor( aMonitor ), m_job( aJob )
 {
@@ -215,13 +215,14 @@ BACKGROUND_JOBS_MONITOR::BACKGROUND_JOBS_MONITOR() : m_jobListDialog( nullptr )
 }
 
 
-BACKGROUND_JOB* BACKGROUND_JOBS_MONITOR::Create( const wxString& aName )
+std::shared_ptr<BACKGROUND_JOB> BACKGROUND_JOBS_MONITOR::Create( const wxString& aName )
 {
-    BACKGROUND_JOB* job = new BACKGROUND_JOB();
+    std::shared_ptr<BACKGROUND_JOB> job = std::make_shared<BACKGROUND_JOB>();
 
     job->m_name = aName;
     job->m_reporter = std::make_shared<BACKGROUND_JOB_REPORTER>( this, job );
 
+    std::lock_guard<std::shared_mutex> lock( m_mutex );
     m_jobs.push_back( job );
 
     if( m_shownDialogs.size() > 0 )
@@ -229,7 +230,11 @@ BACKGROUND_JOB* BACKGROUND_JOBS_MONITOR::Create( const wxString& aName )
         // update dialogs
         for( BACKGROUND_JOB_LIST* list : m_shownDialogs )
         {
-            list->Add( job );
+            list->CallAfter(
+                    [=]()
+                    {
+                        list->Add( job );
+                    } );
         }
     }
 
@@ -237,7 +242,7 @@ BACKGROUND_JOB* BACKGROUND_JOBS_MONITOR::Create( const wxString& aName )
 }
 
 
-void BACKGROUND_JOBS_MONITOR::Remove( BACKGROUND_JOB* aJob )
+void BACKGROUND_JOBS_MONITOR::Remove( std::shared_ptr<BACKGROUND_JOB> aJob )
 {
     if( m_shownDialogs.size() > 0 )
     {
@@ -245,12 +250,17 @@ void BACKGROUND_JOBS_MONITOR::Remove( BACKGROUND_JOB* aJob )
 
         for( BACKGROUND_JOB_LIST* list : m_shownDialogs )
         {
-            list->Remove( aJob );
+            list->CallAfter(
+                    [=]()
+                    {
+                        list->Remove( aJob );
+                    } );
         }
     }
 
+    std::lock_guard<std::shared_mutex> lock( m_mutex );
     m_jobs.erase( std::remove_if( m_jobs.begin(), m_jobs.end(),
-                                  [&]( BACKGROUND_JOB* job )
+                                  [&]( std::shared_ptr<BACKGROUND_JOB> job )
                                {
                                       return job == aJob;
                                } ) );
@@ -259,12 +269,14 @@ void BACKGROUND_JOBS_MONITOR::Remove( BACKGROUND_JOB* aJob )
     {
         for( KISTATUSBAR* statusBar : m_statusBars )
         {
-            statusBar->HideBackgroundProgressBar();
+            statusBar->CallAfter(
-            statusBar->SetBackgroundStatusText( wxT( "" ) );
+                    [=]()
+                    {
+                        statusBar->HideBackgroundProgressBar();
+                        statusBar->SetBackgroundStatusText( wxT( "" ) );
+                    } );
         }
     }
-
-    delete aJob;
 }
 
 
@@ -286,11 +298,15 @@ void BACKGROUND_JOBS_MONITOR::ShowList( wxWindow* aParent, wxPoint aPos )
 {
     BACKGROUND_JOB_LIST* list = new BACKGROUND_JOB_LIST( aParent, aPos );
 
-    for( BACKGROUND_JOB* job : m_jobs )
+    std::shared_lock<std::shared_mutex> lock( m_mutex, std::try_to_lock );
+
+    for( std::shared_ptr<BACKGROUND_JOB> job : m_jobs )
     {
         list->Add( job );
     }
 
+    lock.unlock();
+
     m_shownDialogs.push_back( list );
 
     list->Bind( wxEVT_CLOSE_WINDOW, &BACKGROUND_JOBS_MONITOR::onListWindowClosed, this );
@@ -303,11 +319,12 @@ void BACKGROUND_JOBS_MONITOR::ShowList( wxWindow* aParent, wxPoint aPos )
 }
 
 
-void BACKGROUND_JOBS_MONITOR::jobUpdated( BACKGROUND_JOB* aJob )
+void BACKGROUND_JOBS_MONITOR::jobUpdated( std::shared_ptr<BACKGROUND_JOB> aJob )
 {
+    std::shared_lock<std::shared_mutex> lock( m_mutex, std::try_to_lock );
+
     // this method is called from the reporters from potentially other threads
     // we have to guard ui calls with CallAfter
-
     if( m_jobs.size() > 0 )
     {
         //for now, we go and update the status bar if its the first job in the vector
@@ -328,6 +345,7 @@ void BACKGROUND_JOBS_MONITOR::jobUpdated( BACKGROUND_JOB* aJob )
         }
     }
 
+
     for( BACKGROUND_JOB_LIST* list : m_shownDialogs )
     {
         list->CallAfter(

include/background_jobs_monitor.h

@@ -28,6 +28,7 @@
 #include <widgets/progress_reporter_base.h>
 #include <functional>
 #include <memory>
+#include <shared_mutex>
 #include <vector>
 
 class PROGRESS_REPORTER;
@@ -43,7 +44,8 @@ class wxCloseEvent;
 class BACKGROUND_JOB_REPORTER : public PROGRESS_REPORTER_BASE
 {
 public:
-    BACKGROUND_JOB_REPORTER( BACKGROUND_JOBS_MONITOR* aMonitor, BACKGROUND_JOB* aJob );
+    BACKGROUND_JOB_REPORTER( BACKGROUND_JOBS_MONITOR*        aMonitor,
+                             std::shared_ptr<BACKGROUND_JOB> aJob );
 
     void SetTitle( const wxString& aTitle ) override
     {
@@ -61,7 +63,7 @@ private:
     bool updateUI() override;
 
     BACKGROUND_JOBS_MONITOR* m_monitor;
-    BACKGROUND_JOB*          m_job;
+    std::shared_ptr<BACKGROUND_JOB> m_job;
     wxString m_title;
     wxString m_report;
 };
@@ -92,12 +94,12 @@ public:
      *
      * @param aName is the displayed title for the event
      */
-    BACKGROUND_JOB* Create( const wxString& aName );
+    std::shared_ptr<BACKGROUND_JOB> Create( const wxString& aName );
 
     /**
      * Removes the given background job from any lists and frees it
      */
-    void Remove( BACKGROUND_JOB* job );
+    void Remove( std::shared_ptr<BACKGROUND_JOB> job );
 
     /**
      * Shows the background job list
@@ -123,14 +125,22 @@ private:
     /**
      * Handles job status updates, intended to be called by BACKGROUND_JOB_REPORTER only
      */
-    void jobUpdated( BACKGROUND_JOB* aJob );
+    void jobUpdated( std::shared_ptr<BACKGROUND_JOB> aJob );
 
     BACKGROUND_JOB_LIST* m_jobListDialog;
 
-    std::vector<BACKGROUND_JOB*> m_jobs;
+    /**
+     * Holds a reference to all active background jobs
+     * Access to this vector should be protected by locks since threads may Create or Remove at will
+     * to register their activity
+     */
+    std::vector<std::shared_ptr<BACKGROUND_JOB>> m_jobs;
     std::vector<BACKGROUND_JOB_LIST*> m_shownDialogs;
 
     std::vector<KISTATUSBAR*> m_statusBars;
+
+    /// Mutex to protect access to the m_jobs vector
+    mutable std::shared_mutex m_mutex;
 };
 
 #endif

kicad/pcm/pcm.h

@@ -401,7 +401,7 @@ private:
     std::function<void( int )>                 m_availableUpdateCallback;
     std::thread                                m_updateThread;
 
-    BACKGROUND_JOB*                       m_updateBackgroundJob;
+    std::shared_ptr<BACKGROUND_JOB>              m_updateBackgroundJob;
 };
 
 #endif // PCM_H_