reapack

commit 2a143f4805d100e0a1e0e65c0693194c40c82146
parent 6904837921b0d34a10ba150f8441fc85e6525479
Author: cfillion <cfillion@users.noreply.github.com>
Date:   Sun,  6 Dec 2015 15:42:12 -0500

rewrite thread synchronization logic for downloads

Diffstat:
M
src/download.cpp
 | 
95
+++++++++++++++++++++++++++++++++++++------------------------------------------
M
src/download.hpp
 | 
22
++++++++++++----------

2 files changed, 56 insertions(+), 61 deletions(-)
diff --git a/src/download.cpp b/src/download.cpp
@@ -6,27 +6,23 @@
 
 using namespace std;
 
-vector<Download *> Download::s_active;
+Download::Queue Download::s_finished;
+WDL_Mutex Download::s_mutex;
 
 static const int DOWNLOAD_TIMEOUT = 5;
 
 Download::Download(const string &name, const string &url)
-  : m_threadHandle(0), m_name(name), m_url(url)
+  : m_name(name), m_url(url), m_threadHandle(0)
 {
   reset();
 }
 
 Download::~Download()
 {
-  if(!isFinished())
-    s_active.erase(remove(s_active.begin(), s_active.end(), this));
-
-  // call stop after removing from the active list to prevent
-  // bad access from timeTick -> finishInMainThread
-  stop();
-
-  // free the content buffer
-  reset();
+  if(m_threadHandle) {
+    WaitForSingleObject(m_threadHandle, INFINITE);
+    CloseHandle(m_threadHandle);
+  }
 }
 
 void Download::reset()
@@ -39,25 +35,14 @@ void Download::reset()
 
 void Download::TimerTick()
 {
-  vector<Download *> &activeDownloads = Download::s_active;
-  const size_t size = activeDownloads.size();
-  const auto begin = activeDownloads.begin();
+  Download *dl = Download::NextFinished();
 
-  for(size_t i = 0; i < size; i++) {
-    Download *download = activeDownloads[i];
-
-    if(!download->isFinished())
-      continue;
-
-    // this need to be done before finishInMainThread in case one
-    // of the callbacks deletes the download object
-    activeDownloads.erase(begin + i);
-
-    download->finishInMainThread();
+  if(!dl) {
+    plugin_register("-timer", (void*)TimerTick);
+    return;
   }
 
-  if(Download::s_active.empty())
-    plugin_register("-timer", (void*)TimerTick);
+  dl->finishInMainThread();
 }
 
 void Download::start()
@@ -67,29 +52,11 @@ void Download::start()
 
   reset();
 
-  s_active.push_back(this);
   m_onStart();
 
-  plugin_register("timer", (void*)TimerTick);
-
   m_threadHandle = CreateThread(NULL, 0, Worker, (void *)this, 0, 0);
 }
 
-void Download::stop()
-{
-  if(!m_threadHandle)
-    return;
-
-  abort();
-
-  WaitForSingleObject(m_threadHandle, INFINITE);
-  CloseHandle(m_threadHandle);
-  m_threadHandle = 0;
-
-  // do not call reset() here, m_finished must stay to true
-  // otherwise the callback will not be called
-};
-
 DWORD WINAPI Download::Worker(void *ptr)
 {
   Download *download = static_cast<Download *>(ptr);
@@ -154,24 +121,51 @@ size_t Download::WriteData(char *ptr, size_t rawsize, size_t nmemb, void *data)
   return size;
 }
 
+void Download::MarkAsFinished(Download *dl)
+{
+  WDL_MutexLock lock(&s_mutex);
+
+  // I hope it's OK to call plugin_register from another thread
+  // if it's not this call should be moved to start() and
+  // we should unregister the timer in finishInMainThread()
+  // instead of TimerTick()
+  if(s_finished.empty())
+    plugin_register("timer", (void*)TimerTick);
+
+  s_finished.push(dl);
+}
+
+Download *Download::NextFinished()
+{
+  WDL_MutexLock lock(&s_mutex);
+
+  if(s_finished.empty())
+    return 0;
+
+  Download *dl = s_finished.front();
+  s_finished.pop();
+
+  return dl;
+}
 
 void Download::finish(const int status, const string &contents)
 {
+  // called from the worker thread
+
   WDL_MutexLock lock(&m_mutex);
 
-  // called from the worker thread
   m_finished = true;
   m_status = status;
   m_contents = contents;
+
+  MarkAsFinished(this);
 }
 
 void Download::finishInMainThread()
 {
   WDL_MutexLock lock(&m_mutex);
 
-  // always called from the main thread from timerTick when m_finished is true
-
-  if(m_threadHandle && m_finished) {
+  if(m_threadHandle) {
     CloseHandle(m_threadHandle);
     m_threadHandle = 0;
   }
@@ -204,8 +198,7 @@ DownloadQueue::~DownloadQueue()
 {
   while(!m_queue.empty()) {
     Download *download = m_queue.front();
-    download->stop();
-    // delete called in the stop callback
+    delete download;
 
     m_queue.pop();
   }
diff --git a/src/download.hpp b/src/download.hpp
@@ -3,7 +3,6 @@
 
 #include <queue>
 #include <string>
-#include <vector>
 
 #include <boost/signals2.hpp>
 #include <WDL/mutex.h>
@@ -12,6 +11,7 @@
 
 class Download {
 public:
+  typedef std::queue<Download *> Queue;
   typedef boost::signals2::signal<void ()> Signal;
   typedef Signal::slot_type Callback;
 
@@ -30,10 +30,13 @@ public:
   void onFinish(const Callback &callback) { m_onFinish.connect(callback); }
 
   void start();
-  void stop();
+  void abort();
 
 private:
-  static std::vector<Download *> s_active;
+  static WDL_Mutex s_mutex;
+  static Queue s_finished;
+  static Download *NextFinished();
+  static void MarkAsFinished(Download *);
 
   static void TimerTick();
   static size_t WriteData(char *, size_t, size_t, void *);
@@ -41,22 +44,21 @@ private:
 
   void finish(const int status, const std::string &contents);
   void finishInMainThread();
-  void abort();
   void reset();
 
+  std::string m_name;
+  std::string m_url;
+
+  WDL_Mutex m_mutex;
+
   HANDLE m_threadHandle;
   bool m_aborted;
   bool m_finished;
   int m_status;
   std::string m_contents;
 
-  std::string m_name;
-  std::string m_url;
-
   Signal m_onStart;
   Signal m_onFinish;
-
-  WDL_Mutex m_mutex;
 };
 
 class DownloadQueue {
@@ -76,7 +78,7 @@ public:
   void onPush(const Callback &callback) { m_onPush.connect(callback); }
 
 private:
-  std::queue<Download *> m_queue;
+  Download::Queue m_queue;
 
   Signal m_onPush;
 };