Issue 5179 - Implement asynchronous executor with a controllable lifetime

src/Platform.cpp

Index: src/Platform.cpp
diff --git a/src/Platform.cpp b/src/Platform.cpp
index 17cc7ab37930df839c1de2500b4c07df5781597a..e53ae974a75efeec09e8ac42adc629adda5a47b6 100644
--- a/src/Platform.cpp
+++ b/src/Platform.cpp
@@ -18,6 +18,7 @@
 #include <AdblockPlus/JsEngine.h>
 #include <AdblockPlus/FilterEngine.h>
 #include <AdblockPlus/DefaultLogSystem.h>
+#include <AdblockPlus/AsyncExecutor.h>
 #include "DefaultTimer.h"
 #include "DefaultWebRequest.h"
 #include "DefaultFileSystem.h"
@@ -27,11 +28,6 @@ using namespace AdblockPlus;
 
 namespace
 {
-  void DummyScheduler(const AdblockPlus::SchedulerTask& task)
-  {
-    std::thread(task).detach();
-  }
-
   template<typename T>
   void ValidatePlatformCreationParameter(const std::unique_ptr<T>& param, const char* paramName)
   {
@@ -121,11 +117,35 @@ void Platform::WithLogSystem(const WithLogSystemCallback& callback)
 
 namespace
 {
+  class SharedAsyncExecutor {

[sergei, 2018/02/23 11:16:42]

It's rather a compatibility thing to simplify the updating of libadblockplus in
libadblockplus-android which will already cause a lot of work (because they will
have to figure out what to do with infinitely running operations). In
libadblockplus-android we are currently using
DefaultPlatformBuilder::GetDefaultAsyncExecutor() to obtain a scheduler
(std::function) which is used to safely run asynchronous tasks related to
FilterEngine. In general it's not a public API and under the question to be
changed later.

When DefaultPlatform is being destroyed we invalidate that asynchronous executor
and ensure that no any task is running there, so whatever scheduled by
libadblocklpus-android via the function defaultScheduler will be either able to
normally (without access violations) finish because FilterEngine is not
destroyed yet or ignored at the scheduling stage, what is also safe.

See how it's used with `defaultScheduler` and that it's "invalidated" in the
destructor.

+  public:
+    SharedAsyncExecutor()
+      : executor(new AsyncExecutor())
+    {
+    }
+    void Dispatch(const std::function<void()>& task) {
+      std::lock_guard<std::mutex> lock(asyncExecutorMutex);
+      if (!executor)
+        return;
+      executor->Dispatch(task);
+    }
+    void Invalidate() {
+      std::unique_ptr<AsyncExecutor> tmp;

[sergei, 2018/02/23 11:16:42]

JIC, it's needed to prevent dead locks. If `executor` were destroyed directly
under the `lock` then it could happen that one of the tasks of the `executor` is
waiting for the lock on mutex in order to dispatch a next task and the
`executor` is waiting for the finishing of that task and is not releasing the
mutex.

+      {
+        std::lock_guard<std::mutex> lock(asyncExecutorMutex);
+        tmp = move(executor);
+      }
+    }
+  private:
+    std::mutex asyncExecutorMutex;
+    std::unique_ptr<AsyncExecutor> executor;
+  };
+
   class DefaultPlatform : public Platform
   {
   public:
-    typedef std::shared_ptr<Scheduler> AsyncExecutorPtr;
-    explicit DefaultPlatform(const AsyncExecutorPtr& asyncExecutor, CreationParameters&& creationParams)
+    typedef std::shared_ptr<SharedAsyncExecutor> AsyncExecutorPtr;
+    explicit DefaultPlatform(const std::shared_ptr<SharedAsyncExecutor>& asyncExecutor, CreationParameters&& creationParams)

[hub, 2018/02/23 22:59:35]

Since there is `typedef std::shared_ptr<SharedAsyncExecutor> AsyncExecutorPtr`
above, why isn't AsyncExecutorPtr used here. (and below line 160) ?

[sergei, 2018/03/01 11:18:37]

Fixed, I somehow forgot about it ))

       : Platform(std::move(creationParams)), asyncExecutor(asyncExecutor)
     {
     }
@@ -137,13 +157,13 @@ namespace
     void WithLogSystem(const WithLogSystemCallback&) override;
 
   private:
-    AsyncExecutorPtr asyncExecutor;
+    std::shared_ptr<SharedAsyncExecutor> asyncExecutor;
     std::recursive_mutex interfacesMutex;
   };
 
   DefaultPlatform::~DefaultPlatform()
   {
-    asyncExecutor.reset();
+    asyncExecutor->Invalidate();
     LogSystemPtr tmpLogSystem;
     TimerPtr tmpTimer;
     FileSystemPtr tmpFileSystem;
@@ -184,18 +204,6 @@ namespace
 
 Scheduler DefaultPlatformBuilder::GetDefaultAsyncExecutor()
 {
-  if (!defaultScheduler)
-  {
-    asyncExecutor = std::make_shared<Scheduler>(::DummyScheduler);
-    std::weak_ptr<Scheduler> weakExecutor = asyncExecutor;
-    defaultScheduler = [weakExecutor](const SchedulerTask& task)
-    {
-      if (auto executor = weakExecutor.lock())
-      {
-        (*executor)(task);
-      }
-    };
-  }
   return defaultScheduler;
 }
 
@@ -223,6 +231,11 @@ void DefaultPlatformBuilder::CreateDefaultLogSystem()
 
 std::unique_ptr<Platform> DefaultPlatformBuilder::CreatePlatform()
 {
+  auto sharedAsyncExecutor = std::make_shared<SharedAsyncExecutor>();
+  defaultScheduler = [sharedAsyncExecutor](const SchedulerTask& task)
+  {
+    sharedAsyncExecutor->Dispatch(task);
+  };
   if (!logSystem)
     CreateDefaultLogSystem();
   if (!timer)
@@ -232,7 +245,6 @@ std::unique_ptr<Platform> DefaultPlatformBuilder::CreatePlatform()
   if (!webRequest)
     CreateDefaultWebRequest();
 
-  std::unique_ptr<Platform> platform(new DefaultPlatform(asyncExecutor, std::move(*this)));
-  asyncExecutor.reset();
+  std::unique_ptr<Platform> platform(new DefaultPlatform(sharedAsyncExecutor, std::move(*this)));
   return platform;
 }