Issue #3391 - Add detached initialization class

src/plugin/DetachedInitialization.h

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
- * Copyright (C) 2006-2015 Eyeo GmbH

[Oleksandr, 2016/02/05 07:36:05]

Nit: 2016

[Eric, 2016/02/08 17:34:07]

Done.

+ * Copyright (C) 2006-2016 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.
  *
@@ skipping 166 matching lines @@
     EnsureInitialized();
   }
 
   /**
    * Start an initialization thread if not already initialized and no initialization thread exists.
    *
    * \par Postcondition
    * - if returns, AlreadyStartedInitializer is true
    * - if throws, AlreadyStartedInitializer is false
    */
   void SpawnInitializer()

[Oleksandr, 2016/02/01 02:47:09]

The double checked locking is really convoluted here. Why not just do something
like?

std::once_flag flag;
ThreadMain()
{
  ...
  std::call_once(flag, F);
  cv.notify_all();
}

void SpawnInitializer()
{
  ...
  std::thread th([this]() -> void { ThreadMain(); });
  th.detach();
}

And I would also still use the 

[Eric, 2016/02/03 14:19:01]

[...]
'std::call_once' has different behavior than might be obvious, and it leads to
behavior that does not work as well here.

The main problem is that using 'call_once()' has exception behavior that's
inappropriate for what we need. If you spawn multiple threads and the active
execution thread throws, this function then converts one of the passive threads
into the active execution thread and tries again. In other words, the function
should not be named 'call_once()' but 'call_until_first_return()'. There are
uses where that's appropriate behavior, but this isn't one of them.

Another problem is that using 'call_once()' you have to create one thread per
call to 'SpawnInitializer()'. The present approach only ever creates a single
thread over all instances.

[Oleksandr, 2016/02/05 07:36:05]

I am not convinced converting passive threads into active ones isn't what we
want though. As it is now, re-throwing the exception in EnsureInitialized is
counter intuitive, IMHO. In fact I think we could be better off if we eat the
possible exceptions in the initialization function instead and assume it can not
throw. 
With the usage that this class will have I don't think it will be any
performance hit at all. On the flip side would be a better
readability/maintainability though.

[Eric, 2016/02/08 17:34:07]

There's no other place to throw it. You can't throw it from 'SpawnInitializer()'
since that function returns before the initializer thread even starts. What's
going on here is something like a deferred constructor exception. When
'EnsureInitialized()' throws, it's telling you that it's not initialized and is
never going to be. 
You can never assume that a function call will not throw (even if declared
'noexcept'), because there are a handful of exceptions, such as 'bad_alloc',
that are implicit in the language.

Besides, you can get what you want in the present code just fine. Just use an
initializer function that doesn't throw. That's exactly what I did in
https://codereview.adblockplus.org/29332665/ (which, I must point out, has
_still_ received no comments). 
Threads are reasonably heavy system resources. (Not as much as a process, but
still ...) It's foolish code that spawns a thread just to check some flags and
then terminates, particularly when we've already got code (the present review)
that does that completely generically already.
Because the semantics are different (hard vs. soft errors), this concern isn't
relevant. We need it to behave correctly.

[Eric, 2016/02/08 17:34:07]

It's not what we want, at least not now.

The difference in semantics is that between handling hard errors and soft
errors. If you have hard errors, you don't want to retry. And if your apparatus
does retry, you may need new code in the initialization function not to actually
retry, since some hard errors don't like being tried again.

On the other hand, if you have soft errors, you may want to retry, but you need
to be able to distinguish that the error is hard vs. soft and not combine the
two. We have no capacity to make that distinction yet. We haven't even done a
proper analysis of what error might be soft vs. hard, although the one I've
looked that replace static initialization look all/mostly like hard errors. In
addition, when you want to handle soft errors, you need to have class semantics
to allow an object to exist in a "temporarily not working" state. We don't have
any capacity at present for that either.

The semantics of this present class are for hard errors. This makes everything
easier and we can make incremental progress. If we want to support soft errors
at the outset, it's a lot more code change.

   {
     if (AlreadyInitialized() || AlreadyStartedInitializer())
     {
       return; // fast-return path
     }
     // Assert we have neither completed nor started an initialization thread
     /*
      * Try to obtain a lock, giving us permission to start the initialization thread.
      * try_lock() is not reliable, that is, it can sometimes fail to lock when the mutex is unlocked.
      * It is, however, guaranteed to succeed eventually.
@@ skipping 48 matching lines @@
   {
     EnsureInitializedNoexcept();
     if (initializerException)
     {
       std::rethrow_exception(initializerException);
     }
   }
 };
 
 #endif // _DETACHED_INITIALIZATION_H_