From ee34aab07a5acb870c8a6e192c336e04d92361f5 Mon Sep 17 00:00:00 2001
From: Seth Hillbrand <hillbrand@ucdavis.edu>
Date: Mon, 4 Jun 2018 13:44:43 -0700
Subject: [PATCH] Async hardening

There are three related changes here to harden our handling of threads
in the footprint async loader.

1) Footprint async loader explicitly aborts any remaining loader threads
on exit.

2) We protect the thread join by a mutex

3) We do not pause during no-wait routines
---
 common/footprint_info.cpp            |  7 ++++++
 common/widgets/progress_reporter.cpp |  2 --
 include/footprint_info.h             |  2 ++
 pcbnew/footprint_info_impl.cpp       | 35 +++++++++++++++++-----------
 pcbnew/footprint_info_impl.h         |  1 +
 pcbnew/zone_filler.cpp               |  8 +++----
 6 files changed, 35 insertions(+), 20 deletions(-)

diff --git a/common/footprint_info.cpp b/common/footprint_info.cpp
index c4db568d64..69a70318e9 100644
--- a/common/footprint_info.cpp
+++ b/common/footprint_info.cpp
@@ -147,6 +147,13 @@ FOOTPRINT_ASYNC_LOADER::FOOTPRINT_ASYNC_LOADER() : m_list( nullptr )
 }
 
 
+FOOTPRINT_ASYNC_LOADER::~FOOTPRINT_ASYNC_LOADER()
+{
+    // This is NOP if the load has finished
+    Abort();
+}
+
+
 void FOOTPRINT_ASYNC_LOADER::SetList( FOOTPRINT_LIST* aList )
 {
     m_list = aList;
diff --git a/common/widgets/progress_reporter.cpp b/common/widgets/progress_reporter.cpp
index cc9076801c..346def3c77 100644
--- a/common/widgets/progress_reporter.cpp
+++ b/common/widgets/progress_reporter.cpp
@@ -92,8 +92,6 @@ bool PROGRESS_REPORTER::KeepRefreshing( bool aWait )
     }
     else
     {
-        wxMilliSleep( 20 );
-
         return updateUI();
     }
 }
diff --git a/include/footprint_info.h b/include/footprint_info.h
index e0d36528f4..2ce3cc538d 100644
--- a/include/footprint_info.h
+++ b/include/footprint_info.h
@@ -315,6 +315,8 @@ public:
      */
     FOOTPRINT_ASYNC_LOADER();
 
+    ~FOOTPRINT_ASYNC_LOADER();
+
     /**
      * Assign a FOOTPRINT_LIST to the loader. This does not take ownership of
      * the list.
diff --git a/pcbnew/footprint_info_impl.cpp b/pcbnew/footprint_info_impl.cpp
index 3e7e9919b9..4d9abff17e 100644
--- a/pcbnew/footprint_info_impl.cpp
+++ b/pcbnew/footprint_info_impl.cpp
@@ -39,6 +39,7 @@
 #include <widgets/progress_reporter.h>
 
 #include <thread>
+#include <mutex>
 
 
 void FOOTPRINT_INFO_IMPL::load()
@@ -139,8 +140,8 @@ bool FOOTPRINT_LIST_IMPL::ReadFootprintFiles( FP_LIB_TABLE* aTable, const wxStri
     {
         if( m_progress_reporter && !m_progress_reporter->KeepRefreshing() )
             m_cancelled = true;
-        else
-            wxMilliSleep( 20 );
+
+        wxMilliSleep( 20 );
     }
 
     if( m_cancelled )
@@ -199,29 +200,36 @@ void FOOTPRINT_LIST_IMPL::StartWorkers( FP_LIB_TABLE* aTable, wxString const* aN
 
 void FOOTPRINT_LIST_IMPL::StopWorkers()
 {
+    std::lock_guard<std::mutex> lock1( m_join );
+
     // To safely stop our workers, we set the cancellation flag (they will each
     // exit on their next safe loop location when this is set).  Then we need to wait
     // for all threads to finish as closing the implementation will free the queues
     // that the threads write to.
-    m_cancelled = true;
-
     for( auto& i : m_threads )
         i.join();
 
     m_threads.clear();
     m_queue_in.clear();
     m_count_finished.store( 0 );
-    m_list_timestamp = 0;
+
+    // If we have cancelled in the middle of a load, clear our timestamp to re-load next time
+    if( m_cancelled )
+        m_list_timestamp = 0;
 }
 
 bool FOOTPRINT_LIST_IMPL::JoinWorkers()
 {
-    for( auto& i : m_threads )
-        i.join();
+    {
+        std::lock_guard<std::mutex> lock1( m_join );
 
-    m_threads.clear();
-    m_queue_in.clear();
-    m_count_finished.store( 0 );
+        for( auto& i : m_threads )
+            i.join();
+
+        m_threads.clear();
+        m_queue_in.clear();
+        m_count_finished.store( 0 );
+    }
 
     size_t total_count = m_queue_out.size();
 
@@ -287,8 +295,8 @@ bool FOOTPRINT_LIST_IMPL::JoinWorkers()
     {
         if( m_progress_reporter && !m_progress_reporter->KeepRefreshing() )
             m_cancelled = true;
-        else
-            wxMilliSleep( 20 );
+
+        wxMilliSleep( 20 );
     }
 
     for( auto& thr : threads )
@@ -325,6 +333,5 @@ FOOTPRINT_LIST_IMPL::FOOTPRINT_LIST_IMPL() :
 
 FOOTPRINT_LIST_IMPL::~FOOTPRINT_LIST_IMPL()
 {
-    for( auto& i : m_threads )
-        i.join();
+    StopWorkers();
 }
diff --git a/pcbnew/footprint_info_impl.h b/pcbnew/footprint_info_impl.h
index fb38cc3024..5af8c34a2b 100644
--- a/pcbnew/footprint_info_impl.h
+++ b/pcbnew/footprint_info_impl.h
@@ -65,6 +65,7 @@ class FOOTPRINT_LIST_IMPL : public FOOTPRINT_LIST
     long long                m_list_timestamp;
     PROGRESS_REPORTER*       m_progress_reporter;
     std::atomic_bool         m_cancelled;
+    std::mutex               m_join;
 
     /**
      * Call aFunc, pushing any IO_ERRORs and std::exceptions it throws onto m_errors.
diff --git a/pcbnew/zone_filler.cpp b/pcbnew/zone_filler.cpp
index e618852e3a..c3e1d1998a 100644
--- a/pcbnew/zone_filler.cpp
+++ b/pcbnew/zone_filler.cpp
@@ -147,8 +147,8 @@ bool ZONE_FILLER::Fill( std::vector<ZONE_CONTAINER*> aZones, bool aCheck )
     {
         if( m_progressReporter )
             m_progressReporter->KeepRefreshing();
-        else
-            wxMilliSleep( 20 );
+
+        wxMilliSleep( 20 );
     }
 
     for( size_t ii = 0; ii < fillWorkers.size(); ++ii )
@@ -242,8 +242,8 @@ bool ZONE_FILLER::Fill( std::vector<ZONE_CONTAINER*> aZones, bool aCheck )
     {
         if( m_progressReporter )
             m_progressReporter->KeepRefreshing();
-        else
-            wxMilliSleep( 10 );
+
+        wxMilliSleep( 10 );
     }
 
     for( size_t ii = 0; ii < triangulationWorkers.size(); ++ii )