Issue #3595 - Add an actual scheduler; use joined threads for file system

include/AdblockPlus/JsEngine.h

Index: include/AdblockPlus/JsEngine.h
===================================================================
--- a/include/AdblockPlus/JsEngine.h
+++ b/include/AdblockPlus/JsEngine.h
@@ -20,6 +20,7 @@
 
 #include <functional>
 #include <map>
+#include <memory>
 #include <stdexcept>
 #include <stdint.h>
 #include <string>
@@ -73,6 +74,9 @@
    */
   typedef std::shared_ptr<ScopedV8Isolate> ScopedV8IsolatePtr;
 
+  struct ImmediateSingleUseThreadType {}; ///< Marker class for scheduling policy
+  extern const ImmediateSingleUseThreadType ImmediateSingleUseThread; ///< Dummy constant for scheduling policy

[sergei, 2017/01/20 13:08:13]

Since it's not used I would propose to remove any ImmediateSingleUseThread*.

+
   /**
    * JavaScript engine used by `FilterEngine`, wraps v8.
    */
@@ -133,6 +137,25 @@
         const std::string& filename = "");
 
     /**
+     * Schedule a task with these policies:
+     * - timing policy: start execution immediately.

[sergei, 2017/01/20 13:08:12]

I would remove the comment about timing policy because no one can guarantee that
execution will be started immediately.

[Eric, 2017/03/30 17:16:58]

"Immediately" simply means "at the present time, without delay".

I would have thought it was obvious that this was not an assertion about the
real-time behavior of execution, since we are not in an RTOS (real-time
operating system) environment, but apparently not.

+     * - threading policy: run the task in a single-use thread.
+     *
+     * @param task
+     *   Task to execute. All v8 handles should refer to the present engine.
+     * @param ImmediateSingleUseThreadType
+     *   The schedule policy--create a new thread and discard it afterwards.

[sergei, 2017/01/20 13:08:12]

I would remove that parameter because there is no choice and this parameters has
no influence.

[Eric, 2017/03/30 17:16:58]

This parameter is a marker, acting both as documentation and disambiguation by
function signature. There's not a second version of `Schedule()` in the present
code, but there has been in development code. When I've rewritten this function,
I've created a second marker class and another implementation and then moved
over callers from old to new incrementally, which greatly speeds development.
And because it's a parameter, not a new function name, it play well with
template instantiation.

Once the scheduler can be considered finished, then it will be time to get rid
of this argument. Even in the initial series of 12 review, it wasn't time yet.

+     */
+    void Schedule(std::function<void()> task, ImmediateSingleUseThreadType);

[sergei, 2017/01/20 13:08:13]

Although it's expected that task can be copied internally, it would be still
better to pass it as const reference.

[Eric, 2017/03/30 17:16:58]

In the present code, it's passed by value intentionally. We have to invoke the
copy constructor because we'll eventually run it out of scope, and the cleanest
way of doing this is to do it as part of the call.

In later code, I changed this to an rvalue argument. The reason for that is to
put responsibility for allocation inside the scheduler instead of relying upon
its callers to do the right thing. The caller creates an argument and calls it
with std::move. If I had figured this out before, I would have done it that way
from the outset.

[sergei, 2017/04/03 15:35:31]

Acknowledged.

+
+    /**
+     * Block until there are no more tasks running in the scheduler.
+     *
+     * Note: this function does not prevent new tasks from being scheduled.
+     */
+    void WaitForQuietScheduler();
+
+    /**
      * Initiates a garbage collection.
      */
     void Gc();
@@ -253,7 +276,7 @@
   private:
     explicit JsEngine(const ScopedV8IsolatePtr& isolate);
 
-    JsValuePtr GetGlobalObject();
+    JsValuePtr GetGlobalObject();
 
     /// Isolate must be disposed only after disposing of all objects which are
     /// using it.
@@ -264,6 +287,16 @@
     LogSystemPtr logSystem;
     std::unique_ptr<v8::Persistent<v8::Context>> context;
     EventMap eventCallbacks;
+
+    // Forward declaration for PImpl idiom

[sergei, 2017/01/20 13:08:13]

We don't need this comment.

[Eric, 2017/03/30 17:16:58]

Yes, we do. Dangling declarations with zero documentation are always confusing.

+    class SchedulerImpl;
+    /**
+     * Scheduler for tasks executed under the current engine.
+     *
+     * \par Invariant
+     *   `bool(scheduler)` is always true
+     */
+    std::unique_ptr<SchedulerImpl> scheduler;
   };
 }