From 1fadf563688785a51ce4fe935a7865d05e3e72a9 Mon Sep 17 00:00:00 2001
From: Ivo van Dongen <info@ivovandongen.nl>
Date: Tue, 12 Feb 2019 11:43:18 +0200
Subject: [android] change lock scope in run_loop to avoid deadlocks. simplify
 as well

---
 platform/android/src/async_task.cpp    |  1 -
 platform/android/src/run_loop.cpp      | 62 +++++++++++++++-------------------
 platform/android/src/run_loop_impl.hpp |  6 +---
 platform/android/src/timer.cpp         | 12 +++----
 4 files changed, 33 insertions(+), 48 deletions(-)

(limited to 'platform')

diff --git a/platform/android/src/async_task.cpp b/platform/android/src/async_task.cpp
index 7b78eadb0c..6c14e96fa6 100644
--- a/platform/android/src/async_task.cpp
+++ b/platform/android/src/async_task.cpp
@@ -13,7 +13,6 @@ class AsyncTask::Impl : public RunLoop::Impl::Runnable {
 public:
     Impl(std::function<void()>&& fn)
         : queued(true), task(std::move(fn)) {
-        loop->initRunnable(this);
     }
 
     ~Impl() {
diff --git a/platform/android/src/run_loop.cpp b/platform/android/src/run_loop.cpp
index b55f2be190..fc45539e5e 100644
--- a/platform/android/src/run_loop.cpp
+++ b/platform/android/src/run_loop.cpp
@@ -141,55 +141,47 @@ void RunLoop::Impl::wake() {
 }
 
 void RunLoop::Impl::addRunnable(Runnable* runnable) {
-    std::lock_guard<std::recursive_mutex> lock(mtx);
-
-    if (runnable->iter == runnables.end()) {
-        auto iter = runnables.insert(runnables.end(), runnable);
-        runnable->iter = std::move(iter);
+    {
+        std::lock_guard<std::mutex> lock(mutex);
+        runnables.push_back(runnable);
     }
 
     wake();
 }
 
 void RunLoop::Impl::removeRunnable(Runnable* runnable) {
-    std::lock_guard<std::recursive_mutex> lock(mtx);
-
-    if (runnable->iter == runnables.end()) {
-        return;
-    }
-
-    if (nextRunnable == runnable->iter) {
-        ++nextRunnable;
-    }
-
-    runnables.erase(runnable->iter);
-    runnable->iter = runnables.end();
-}
-
-void RunLoop::Impl::initRunnable(Runnable* runnable) {
-    std::lock_guard<std::recursive_mutex> lock(mtx);
-    runnable->iter = runnables.end();
+    std::lock_guard<std::mutex> lock(mutex);
+    runnables.remove(runnable);
 }
 
 Milliseconds RunLoop::Impl::processRunnables() {
-    std::lock_guard<std::recursive_mutex> lock(mtx);
-
     auto now = Clock::now();
     auto nextDue = TimePoint::max();
-
-    // O(N) but in the render thread where we get tons
-    // of messages, the size of the list is usually 1~2.
-    for (nextRunnable = runnables.begin(); nextRunnable != runnables.end();) {
-        Runnable* runnable = *(nextRunnable++);
-
-        auto const dueTime = runnable->dueTime();
-        if (dueTime <= now) {
-            runnable->runTask();
-        } else {
-            nextDue = std::min(nextDue, dueTime);
+    std::list<Runnable*> tmp;
+    {
+        std::lock_guard<std::mutex> lock(mutex);
+
+        // O(N) but in the render thread where we get tons
+        // of messages, the size of the list is usually 1~2.
+        auto it = runnables.begin();
+        while (it != runnables.end()) {
+            Runnable* runnable = *it;
+
+            auto const dueTime = runnable->dueTime();
+            if (dueTime <= now) {
+                tmp.push_back(runnable);
+                runnables.erase(it++);
+            } else {
+                nextDue = std::min(nextDue, dueTime);
+                ++it;
+            }
         }
     }
 
+    for (auto runnable : tmp) {
+        runnable->runTask();
+    }
+
     if (runnables.empty() || nextDue == TimePoint::max()) {
         return Milliseconds(-1);
     }
diff --git a/platform/android/src/run_loop_impl.hpp b/platform/android/src/run_loop_impl.hpp
index 9f4dec476e..0a9ea156fd 100644
--- a/platform/android/src/run_loop_impl.hpp
+++ b/platform/android/src/run_loop_impl.hpp
@@ -26,8 +26,6 @@ public:
 
         virtual void runTask() = 0;
         virtual TimePoint dueTime() const = 0;
-
-        std::list<Runnable*>::iterator iter;
     };
 
     Impl(RunLoop*, RunLoop::Type);
@@ -37,7 +35,6 @@ public:
 
     void addRunnable(Runnable*);
     void removeRunnable(Runnable*);
-    void initRunnable(Runnable*);
 
     Milliseconds processRunnables();
 
@@ -55,9 +52,8 @@ private:
 
     std::unique_ptr<Thread<Alarm>> alarm;
 
-    std::recursive_mutex mtx;
+    std::mutex mutex;
     std::list<Runnable*> runnables;
-    std::list<Runnable*>::iterator nextRunnable = runnables.end();
 };
 
 } // namespace util
diff --git a/platform/android/src/timer.cpp b/platform/android/src/timer.cpp
index 2d9ee49e9b..a45c48702e 100644
--- a/platform/android/src/timer.cpp
+++ b/platform/android/src/timer.cpp
@@ -10,21 +10,20 @@ namespace util {
 
 class Timer::Impl : public RunLoop::Impl::Runnable {
 public:
-    Impl() {
-        loop->initRunnable(this);
-    }
+    Impl() = default;
 
     ~Impl() {
         stop();
     }
 
-    void start(Duration timeout, Duration repeat_, std::function<void ()>&& task_) {
+    void start(Duration timeout, Duration repeat_, std::function<void()>&& task_) {
         stop();
 
         repeat = repeat_;
         task = std::move(task_);
         // Prevent overflows when timeout is set to Duration::max()
-        due = (timeout == Duration::max()) ? std::chrono::time_point<Clock>::max() : Clock::now() + timeout;
+        due = (timeout == Duration::max()) ? std::chrono::time_point<Clock>::max() : Clock::now() +
+                                                                                     timeout;
         loop->addRunnable(this);
     }
 
@@ -59,8 +58,7 @@ private:
     std::function<void()> task;
 };
 
-Timer::Timer()
-    : impl(std::make_unique<Impl>()) {
+Timer::Timer() : impl(std::make_unique<Impl>()) {
 }
 
 Timer::~Timer() = default;
-- 
cgit v1.2.1