From 59e5c836fc26fd07f1cda8d590275c749d223b85 Mon Sep 17 00:00:00 2001 From: Jeff Young Date: Wed, 10 Nov 2021 22:11:52 +0000 Subject: [PATCH] Tighter thread safety for PCM progress dialog. --- kicad/pcm/dialogs/dialog_pcm_progress.cpp | 140 +++++++++++----------- kicad/pcm/dialogs/dialog_pcm_progress.h | 36 +++--- kicad/pcm/pcm_task_manager.cpp | 42 +++---- 3 files changed, 99 insertions(+), 119 deletions(-) diff --git a/kicad/pcm/dialogs/dialog_pcm_progress.cpp b/kicad/pcm/dialogs/dialog_pcm_progress.cpp index bafbf11362..3dde78a66d 100644 --- a/kicad/pcm/dialogs/dialog_pcm_progress.cpp +++ b/kicad/pcm/dialogs/dialog_pcm_progress.cpp @@ -25,7 +25,9 @@ #define GAUGE_RANGE 1000 DIALOG_PCM_PROGRESS::DIALOG_PCM_PROGRESS( wxWindow* parent, bool aShowDownloadSection ) : - DIALOG_PCM_PROGRESS_BASE( parent ) + DIALOG_PCM_PROGRESS_BASE( parent ), + PROGRESS_REPORTER_BASE( 1 ), + m_finished( false ) #if wxCHECK_VERSION( 3, 1, 0 ) , m_appProgressIndicator( parent->GetParent(), GAUGE_RANGE ) @@ -34,14 +36,11 @@ DIALOG_PCM_PROGRESS::DIALOG_PCM_PROGRESS( wxWindow* parent, bool aShowDownloadSe #if wxCHECK_VERSION( 3, 1, 0 ) m_appProgressIndicator.Pulse(); #endif - m_cancelled.store( false ); m_reporter->SetImmediateMode(); m_downloadGauge->SetRange( GAUGE_RANGE ); m_overallGauge->SetRange( GAUGE_RANGE ); - SetOverallProgressPhases( 1 ); - if( !aShowDownloadSection ) m_panelDownload->Hide(); } @@ -49,45 +48,32 @@ DIALOG_PCM_PROGRESS::DIALOG_PCM_PROGRESS( wxWindow* parent, bool aShowDownloadSe void DIALOG_PCM_PROGRESS::OnCancelClicked( wxCommandEvent& event ) { + SetNumPhases( 1 ); + SetOverallProgress( 1, 1 ); + Report( _( "Aborting remaining tasks." ) ); + m_cancelled.store( true ); - m_buttonCancel->Disable(); + m_finished.store( true ); } void DIALOG_PCM_PROGRESS::OnCloseClicked( wxCommandEvent& event ) { - EndModal( wxID_OK ); + m_progress.store( m_maxProgress ); } void DIALOG_PCM_PROGRESS::Report( const wxString& aText, SEVERITY aSeverity ) { - CallAfter( - [=] - { - m_reporter->Report( aText, aSeverity ); - } ); + std::lock_guard guard( m_mutex ); + m_reports.push_back( std::make_pair( aText, aSeverity ) ); } void DIALOG_PCM_PROGRESS::SetDownloadProgress( uint64_t aDownloaded, uint64_t aTotal ) { - if( aDownloaded > aTotal ) - aDownloaded = aTotal; - - int value = 0; - - if( aTotal > 0 ) - value = aDownloaded * GAUGE_RANGE / aTotal; - - CallAfter( - [=] - { - m_downloadText->SetLabel( wxString::Format( _( "Downloaded %lld/%lld Kb" ), - toKb( aDownloaded ), toKb( aTotal ) ) ); - - m_downloadGauge->SetValue( value ); - } ); + m_downloaded.store( std::min( aDownloaded, aTotal ) ); + m_downloadTotal.store( aTotal ); } @@ -99,59 +85,69 @@ uint64_t DIALOG_PCM_PROGRESS::toKb( uint64_t aValue ) void DIALOG_PCM_PROGRESS::SetOverallProgress( uint64_t aProgress, uint64_t aTotal ) { - double current = ( m_currentPhase + aProgress / (double) aTotal ) / m_overallPhases; - - if( current > 1.0 ) - current = 1.0; - - int value = current * GAUGE_RANGE; - - CallAfter( - [=] - { - m_overallGauge->SetValue( value ); -#if wxCHECK_VERSION( 3, 1, 0 ) - m_appProgressIndicator.SetValue( value ); -#endif - } ); -} - - -void DIALOG_PCM_PROGRESS::SetOverallProgressPhases( int aPhases ) -{ - m_currentPhase = 0; - m_overallPhases = aPhases; -} - - -void DIALOG_PCM_PROGRESS::AdvanceOverallProgressPhase() -{ - m_currentPhase++; - SetOverallProgress( 0, 1 ); + m_overallProgress.store( std::min( aProgress, aTotal ) ); + m_overallProgressTotal.store( aTotal ); } void DIALOG_PCM_PROGRESS::SetFinished() { - CallAfter( - [this] - { - m_buttonCancel->Disable(); - m_buttonClose->Enable(); - } ); + m_finished.store( true ); } -void DIALOG_PCM_PROGRESS::SetDownloadsFinished() +bool DIALOG_PCM_PROGRESS::updateUI() { - CallAfter( - [this] - { - m_downloadText->SetLabel( _( "All downloads finished" ) ); - } ); + int phase = m_phase.load(); + int phases = m_numPhases.load(); + double current = m_overallProgress.load() / (double) m_overallProgressTotal.load(); + + if( phases > 0 ) + current = ( phase + current ) / phases; + + if( current > 1.0 ) + current = 1.0; + + m_overallGauge->SetValue( current * GAUGE_RANGE ); +#if wxCHECK_VERSION( 3, 1, 0 ) + m_appProgressIndicator.SetValue( current * GAUGE_RANGE ); +#endif + + if( m_downloadTotal.load() == 0 ) + { + m_downloadText->SetLabel( wxEmptyString ); + m_downloadGauge->SetValue( 0 ); + } + else + { + m_downloadText->SetLabel( wxString::Format( _( "Downloaded %lld/%lld Kb" ), + toKb( m_downloaded ), + toKb( m_downloadTotal ) ) ); + + current = m_downloaded.load() / (double) m_downloadTotal.load(); + + if( current > 1.0 ) + current = 1.0; + + m_downloadGauge->SetValue( current * GAUGE_RANGE ); + } + + std::lock_guard guard( m_mutex ); + + for( const std::pair& pair : m_reports ) + m_reporter->Report( pair.first, pair.second ); + + m_reports.clear(); + + if( m_finished.load() ) + { + m_buttonCancel->Disable(); + m_buttonClose->Enable(); + } + + wxYield(); + + return true; } -bool DIALOG_PCM_PROGRESS::IsCancelled() -{ - return m_cancelled.load(); -} + diff --git a/kicad/pcm/dialogs/dialog_pcm_progress.h b/kicad/pcm/dialogs/dialog_pcm_progress.h index 6a31f68b16..f790318994 100644 --- a/kicad/pcm/dialogs/dialog_pcm_progress.h +++ b/kicad/pcm/dialogs/dialog_pcm_progress.h @@ -27,6 +27,8 @@ #include #if wxCHECK_VERSION( 3, 1, 0 ) #include +#include + #endif @@ -35,11 +37,8 @@ * * This dialog is designed to work with PCM_TASK_MANAGER's threading system. * Some of it's methods are safe to call from a non-UI thread. - * - * SetOverallProgress(), SetOverallProgressPhases() and AdvanceOverallProgressPhase() - * are meant to be called from the same thread. */ -class DIALOG_PCM_PROGRESS : public DIALOG_PCM_PROGRESS_BASE +class DIALOG_PCM_PROGRESS : public DIALOG_PCM_PROGRESS_BASE, public PROGRESS_REPORTER_BASE { protected: // Handlers for DIALOG_PCM_PROGRESS_BASE events. @@ -59,26 +58,25 @@ public: ///< Safe to call from non-UI thread. Sets current overall progress gauge. void SetOverallProgress( uint64_t aProgress, uint64_t aTotal ); - ///< Safe to call from non-UI thread. Sets number of phases for overall progress gauge. - void SetOverallProgressPhases( int aPhases ); - - ///< Safe to call from non-UI thread. Increments current phase by one. - void AdvanceOverallProgressPhase(); - - ///< Thread safe. Updates download text. - void SetDownloadsFinished(); - ///< Thread safe. Disables cancel button, enables close button. void SetFinished(); - ///< Thread safe. Return true if cancel was clicked. - bool IsCancelled(); +private: + bool updateUI() override; + + static uint64_t toKb( uint64_t aValue ); private: - static uint64_t toKb( uint64_t aValue ); - int m_currentPhase; - int m_overallPhases; - std::atomic_bool m_cancelled; + std::atomic_int64_t m_downloaded; + std::atomic_int64_t m_downloadTotal; + + std::atomic_int64_t m_overallProgress; + std::atomic_int64_t m_overallProgressTotal; + + std::atomic_bool m_finished; + + std::vector< std::pair > m_reports; + #if wxCHECK_VERSION( 3, 1, 0 ) wxAppProgressIndicator m_appProgressIndicator; #endif diff --git a/kicad/pcm/pcm_task_manager.cpp b/kicad/pcm/pcm_task_manager.cpp index 2c6c46c90f..b5f8ca8e5f 100644 --- a/kicad/pcm/pcm_task_manager.cpp +++ b/kicad/pcm/pcm_task_manager.cpp @@ -23,8 +23,6 @@ #include "kicad_curl/kicad_curl_easy.h" #include "pcm_task_manager.h" -#include "paths.h" -#include "picosha2.h" #include "reporter.h" #include "wxstream_helper.h" @@ -33,7 +31,6 @@ #include #include #include -#include #include #include #include @@ -87,14 +84,13 @@ void PCM_TASK_MANAGER::DownloadAndInstall( const PCM_PACKAGE& aPackage, const wx if( !hash_match ) { - m_reporter->Report( - wxString::Format( - _( "Downloaded archive hash for package %s does not match " - "repository entry. This may indicate a problem with the " - "package, if the issue persists report this to repository " - "maintainers." ), - aPackage.identifier ), - RPT_SEVERITY_ERROR ); + m_reporter->Report( wxString::Format( _( "Downloaded archive hash for package " + "%s does not match repository entry. " + "This may indicate a problem with the " + "package, if the issue persists " + "report this to repository maintainers." ), + aPackage.identifier ), + RPT_SEVERITY_ERROR ); } else { @@ -136,13 +132,10 @@ int PCM_TASK_MANAGER::downloadFile( const wxString& aFilePath, const wxString& u TRANSFER_CALLBACK callback = [&]( size_t dltotal, size_t dlnow, size_t ultotal, size_t ulnow ) { if( dltotal > 1024 ) - { m_reporter->SetDownloadProgress( dlnow, dltotal ); - } else - { m_reporter->SetDownloadProgress( 0.0, 0.0 ); - } + return m_reporter->IsCancelled(); }; @@ -340,7 +333,7 @@ void PCM_TASK_MANAGER::InstallFromFile( wxWindow* aParent, const wxString& aFile m_pcm->MarkInstalled( package, package.versions[0].version, "" ); m_reporter->SetFinished(); - m_reporter->ShowModal(); + m_reporter->KeepRefreshing( true ); m_reporter->Destroy(); m_reporter.reset(); } @@ -393,9 +386,11 @@ void PCM_TASK_MANAGER::RunQueue( wxWindow* aParent ) { m_reporter = std::make_unique( aParent ); - m_reporter->SetOverallProgressPhases( m_download_queue.size() + m_install_queue.size() ); + m_reporter->SetNumPhases( m_download_queue.size() + m_install_queue.size() ); m_reporter->Show(); + wxSafeYield(); + std::mutex mutex; std::condition_variable condvar; bool download_complete = false; @@ -411,8 +406,6 @@ void PCM_TASK_MANAGER::RunQueue( wxWindow* aParent ) condvar.notify_all(); } - m_reporter->SetDownloadsFinished(); - std::unique_lock lock( mutex ); download_complete = true; condvar.notify_all(); @@ -439,7 +432,7 @@ void PCM_TASK_MANAGER::RunQueue( wxWindow* aParent ) PCM_TASK task; m_install_queue.pop( task ); task(); - m_reporter->AdvanceOverallProgressPhase(); + m_reporter->AdvancePhase(); } lock.lock(); @@ -447,19 +440,12 @@ void PCM_TASK_MANAGER::RunQueue( wxWindow* aParent ) } while( ( !m_install_queue.empty() || !download_complete ) && !m_reporter->IsCancelled() ); - if( m_reporter->IsCancelled() ) - { - m_reporter->SetOverallProgressPhases( 1 ); - m_reporter->SetOverallProgress( 1, 1 ); - m_reporter->Report( _( "Aborting remaining tasks." ) ); - } - m_reporter->Report( _( "Done." ) ); m_reporter->SetFinished(); } ); - m_reporter->ShowModal(); + m_reporter->KeepRefreshing( true ); m_reporter->Destroy(); m_reporter.reset();