From 34a13cb0c589a23525c6ff8512d672d0d06b8f3f Mon Sep 17 00:00:00 2001
From: Marek Roszko <mark.roszko@gmail.com>
Date: Fri, 11 Aug 2023 22:37:43 -0400
Subject: [PATCH] Add more thread safety to background jobs
Fixes https://gitlab.com/kicad/code/kicad/-/issues/15395
---
common/background_jobs_monitor.cpp | 58 +++++++++++++++++++-----------
include/background_jobs_monitor.h | 22 ++++++++----
kicad/pcm/pcm.h | 2 +-
3 files changed, 55 insertions(+), 27 deletions(-)
diff --git a/common/background_jobs_monitor.cpp b/common/background_jobs_monitor.cpp
index 4c925fb036..e7c9e33151 100644
--- a/common/background_jobs_monitor.cpp
+++ b/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->SetBackgroundStatusText( wxT( "" ) );
+ statusBar->CallAfter(
+ [=]()
+ {
+ 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(
diff --git a/include/background_jobs_monitor.h b/include/background_jobs_monitor.h
index 9bb92de106..006a70a364 100644
--- a/include/background_jobs_monitor.h
+++ b/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
\ No newline at end of file
diff --git a/kicad/pcm/pcm.h b/kicad/pcm/pcm.h
index 298f3a5db7..e901e5800f 100644
--- a/kicad/pcm/pcm.h
+++ b/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_