Issue #2230, #3391 - Load filters on "download begin" event

src/plugin/DetachedInitialization.h

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 #ifndef _DETACHED_INITIALIZATION_H_
 #define _DETACHED_INITIALIZATION_H_
 
 #include <atomic>
 #include <thread>
 #include <mutex>
 #include <chrono>
 #include <condition_variable>
 
 /**
- * A detached initializer for static class members
+ * Initialization performed in a detached thread
  *
- * Detached initializers use a separate thread to avoid delays at instantiation.

[sergei, 2016/01/29 10:03:20]

This line is OK.

- * Variables statically initialized in a DLL cannot, however, contain threads.
- * Thus such initialization must occur in the constructor at the earliest.

[sergei, 2016/01/29 10:03:20]

These two lines are not related to this file.

- *

[sergei, 2016/01/29 10:03:20]

It's important to say in the description whether the class is thread-safe, thus
whether it's supposed that all methods may be called "simultaneously" from
several threads.

BTW, what happens if somebody calls SpawnInitializer() and EnsureInitialized()
"simultaneously"? If we cannot spawn a thread (I hope it's very unlikely) then
the waiter will wait forever, right?

- * This kind of initialization acts shares much in common with a singleton.
- * Rather than constructing a singleton, instead we execute an initialization function.
+ * This kind of initialization shares much in common with a singleton.
+ * Rather than constructing a singleton, instead we execute an initialization function exactly once.

[sergei, 2016/01/29 10:03:19]

There is nothing about singleton.
"Detached initializers use a separate thread to avoid delays at instantiation."
looks completely sufficient.

  * The multiprocessing and synchronization issues, however, are essentially identical.

[sergei, 2016/01/29 10:03:20]

This line is not related to this file.

- *
- * The designated usage of this class:
- * 1) Define a static class member 'x' of this type with an initializer function.
- * 2) Call 'x.SpawnInitializer()' in the constructors to run the initializer.
- * 3) Call 'x.EnsureInitialized()' before relying upon any of the side effects of the initializer.
- *
  * The implementation acts much like a multithreaded singleton initialization using a double-checked lock.
  * See https://en.wikipedia.org/wiki/Double-checked_locking and http://preshing.com/20130922/acquire-and-release-fences/

[sergei, 2016/01/29 10:03:20]

I would say that semantically it looks completely differently. One can notice
some similarity in the implementation but I would say that comment is rather
confusing.

  * The flag members of this class enable fast-return paths, that is, a return without obtaining a mutex lock.

[sergei, 2016/01/29 10:03:19]

I don't think we need this line.

  *
- * \tparam F
- *   The initializer function
+ * Designated usage:
+ * 1) Derive from this class, defining the factory Initializer().

[sergei, 2016/01/29 10:03:19]

I am afraid, I disagree with such approach (even beside the virtual
initializer).
The main issue is that it's very easy to forget to call `EnsureInitialized()`
from the destructor of the class (as it happens here) what leads to the race
condition, the derived class with all members can be already destroyed but the
initializing function using them is still running.

+ * 2) Call 'SpawnInitializer()' in the constructors.
+ * 3) Call 'EnsureInitialized()' before relying upon any of the side effects of the initializer.
  *
  * \par Serialization Guarantees
  * The visible effect of these function returns are serialized: 
  *   - SpawnInitializer() before ThreadMain()
  *   - ThreadMain() before EnsureInitialized()
  *   - If ThreadMain() started, ThreadMain() before destructor
  */
-template<void (&F)()>
 class DetachedInitializer
 {
   // "protected" declaration allows white-box subclass for testing
 protected:
   /**
    * Set to true after initializer thread successfully constructed; remains true thereafter.
    */
   std::atomic<bool> hasInitializerStarted;
 
   /**
@@ skipping 29 matching lines @@
   inline bool AlreadyInitialized() // noexcept
   {
     // Memory fence ensures load starts after any other load or store
     return isInitializationComplete.load(std::memory_order_acquire);
   }
 
   inline bool AlreadyStartedInitializer() // noexcept
   {
     return hasInitializerStarted.load(std::memory_order_acquire);
   }
+  
+  /**
+   * Type of initializer objects
+   *
+   * Note that the underlying function returns 'void'.
+   * This class refuses responsibility for specific error semantics.
+   * If the initializer thread throws, then so will EnsureInitialized().

[sergei, 2016/01/29 10:03:19]

Only this line is important, the rest about void and specific error semantics
looks redundant.

+   * Anything beyond this is the responsibility of the owner.
+   */
+  typedef std::function<void()> InitializerType;
 
   /**
-   * Main function for thread
+   * Factory function for initializers
    */
-  void ThreadMain()
+  virtual InitializerType Initializer() = 0;
+
+  /**
+   * Main function for initialization thread
+   */
+  void ThreadMain(InitializerType f)
   {
     try
     {
-      F();
+      f();
     }
-    catch (std::exception& e)
+    catch (...)
     {
       // Assert initializer threw an exception
       initializerException = std::current_exception();
     }
     /*
      * Synchronize with reading the completion status in EnsureInitialized().
      * Serialize return from SpawnInitializer() and this function.

[sergei, 2016/01/29 10:03:20]

The comment is wrong. We need to lock mutex here mainly to protect condition
variable as well as it's a common convenient pattern of using of flag, condition
variable and mutex.
I would remove it completely.

      */
     LockGuardType lg(mutex);
     isInitializationComplete.store(true, std::memory_order_release);
     cv.notify_all();
   }
 
   /**
    * "noexcept" version of EnsureInitialized() used to implement both it and the destructor
    */
   void EnsureInitializedNoexcept() // noexcept
   {
     try
     {
       if (AlreadyInitialized())
       {
         return; // fast-return path

[sergei, 2016/01/29 10:03:19]

I would say we don't need these comments "fast return path".

       }
       UniqueLockType ul(mutex); // won't throw because this class uses only scoped locks
       /*
        * We must always check initialization status under lock.
        * The loop protects against spurious wake-up calls.
        */
       while (!AlreadyInitialized())
       {
         cv.wait(ul); // won't throw because mutex is locked
       }
@@ skipping 60 matching lines @@
          */
         if (AlreadyStartedInitializer())
         {
           return;
         }
         /*
          * This thread constructor is the only statement that can throw in this function.
          * If it does throw, the initialization-started flag won't be set.
          * An exception here is thus a soft error, since a future invocation might succeed.
          */
-        std::thread th([this]() -> void { ThreadMain(); });
+        std::thread th(std::bind([this](InitializerType f) -> void { ThreadMain(f); }, Initializer()));
         // Memory fence ensures store ends before any other load or store
         hasInitializerStarted.store(true, std::memory_order_release);
         th.detach(); // Won't throw because the thread is both valid and joinable
         return;
       }
       else
       {
         // Assert mutex is not locked
         if (AlreadyStartedInitializer())
         {
@@ skipping 17 matching lines @@
   void EnsureInitialized()
   {
     EnsureInitializedNoexcept();
     if (initializerException)
     {
       std::rethrow_exception(initializerException);
     }
   }
 };
 
+/**
+ * Detached initializers use a separate thread to avoid delays at instantiation.
+ * The constructors of variables statically initialized in a DLL cannot, however, contain threads.
+ * Thus such initialization must (generally) occur in the constructor.
+ */
+template<void(&F)()>
+class DetachedInitializerStaticFunction
+  : public DetachedInitializer
+{
+  InitializerType Initializer() override
+  {
+    return InitializerType([] { F(); });
+  }
+};
+
 #endif // _DETACHED_INITIALIZATION_H_