Issue #1234, #2058 - Rewrite log facility, improving thread implementation

Issue #1234, #2058 - Rewrite log facility, improving thread implementation

Replaced old class 'LogQueue' definition with an entirely new one based on the
class 'ActiveQueue'. Added class 'LogQueueReference' to substitute for static
initialization.

Eliminate explicit use of process and thread ID outside the class 'LogEntry'.
All log entries are consistently initialized with this information.

Add location information to 'LogEntry'. This includes both the old pattern of
numeric identifiers and a new one of source code identifiers.

Added ActiveQueue.h and Placeholder.h, implementing a synchronized queue and
an active queue, that is, one with an autonomous consumer. Added tests for
these classes, which are segregated into their own project (for the time
being) to enable development of the tests with a more recent toolset.

Removed PluginClientBase.{h,cpp}, whose only remaining code was for the old
log queue and its elements. Subsequently removed code only used therein.
Removed CPluginSettings' member functions 'HasInstance()' and 'AddError()'.
Removed 'CPluginTab' member variables 'm_thread', 'm_isActivated', and
'm_continueThreadRunning', member functions 'ThreadProc()', 'OnActivate()',
'OnUpdate()', and class function 'TabThreadProc()'.

Removed unneeded SELFTEST macros.

[sergei, 2015-10-01 15:50:08]

Firstly, I have quickly gone through src/plugin/ActiveQueue.h file and have some
comments there.

Secondly, is it possible to split it into several codereviews, like cleaning
(headers), the changes in LogEntry and adding location, refactoring of logging
(ActiveQueue and so on)?

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
File src/plugin/ActiveQueue.h (right):

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:25: * A synchronized FIFO queue with thread notice on
receipt.
So, why is it called MessageQueue? It seems better to name is
'SynchronizedDeque'. Pay attention that I have used `Deque` because soon or late
it will be deque, it uses deque internally and I've proposed `push_front` in the
comments.
As well as I don't like the exposing of methods which deal only with
synchronization primitives.

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:42: MessageQueue() {} // = default
Why do we need `= default`? I would remove it.

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:53: void Insert(const T& t)
Do you mind to rename it to `push_back` or `PushBack`?

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:55: {
This additional scope is not needed here nor in the `Insert(T&&)` method.

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:83: void Rouse(Function& f)
The reason we need such method is to set a value of `ActiveQueue:::running`. We
can achieve it with having `push_front` method here which looks more appropriate
as a member of `MessageQueue` class.

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:102: bool Remove(Function& f)
Why not to call it `pop_front` or `try_pop_front(... timeout)` and make this
method waiting for the element in the queue?

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:112: Placeholder<T> x;
I see only one reason to use a class `Placeholder` which is an optimization of
storing the object on the stack. If it's really needed we can add it later, for
now I would proceed with `std::unique_ptr` here.

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:131: bool WaitIf(Predicate& p)
It's still `MessageQueue` class, how is this method related with a queue nature
of it? It seems better to have a method which waits until any element is added
into the queue.
Actually such waiting can be done in `Remove` method.

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:158: WAIT_FOR_RETURN_TYPE WaitFor(const
std::chrono::duration<R,P>& relativeDuration)
Why do we need this method? It seems to be not used and it's definitely has no
relation to the queue nature.

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:180: class ActiveQueue
I would like to get rid of template parameters T and F and make it working only
with `std::function<void()>` as T. Having that the code will be more reliable
and easier to understand. If it is a reason of any sensitive performance issue
we can add them afterwards.

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:191: bool running;
Hint: if we access it only from the working thread then we don't need any
protection.

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:196: F& processor;
In general it's a bad idea to use reference member type, especially in so
generic class. Even more, in C++ it's usually assumed that a functor can be
copied any number of times, so it's a burden of the caller to make the proper
memory management. But as pointed above, I think we can just get rid from this
member.

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:225: while (
It means that it will run until the queue is not empty even if `running` is
already false. Is it intended? I would rather stop the loop and clear the queue.

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/Placehol...
File src/plugin/Placeholder.h (right):

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/Placehol...
src/plugin/Placeholder.h:35: class Placeholder
The destructor is missing.

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/PluginDe...
File src/plugin/PluginDebug.h (right):

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/PluginDe...
src/plugin/PluginDebug.h:132: static pointer queueMasterReference;
Why do we need more static stuff? We have already singletons for example
CAdblockPlusClient and CPluginSettings, why not to combine them all in one
singleton?

[Eric, 2015-10-08 21:05:41]

On 2015/10/01 15:50:08, sergei wrote:
> Firstly, I have quickly gone through src/plugin/ActiveQueue.h file and have
some
> comments there.

I have read and replied to all these comments. There are no defect reports.
There's a couple bits of extra scope that are purely a style issue.

Please finish the review so that we can move forward.

> Secondly, is it possible to split it into several codereviews, like cleaning
> (headers), the changes in LogEntry and adding location, refactoring of logging
> (ActiveQueue and so on)?

I won't be splitting this up. In fact, this is but one part of a reworking of
the log facility that I've already separated out. The last time I introduced an
abstract class before its use, I got slammed for proposing something useless. So
I learned my lesson and now the abstract class ("ActiveQueue") is in the same
review as where it's used (the log queue).

As for the most of the other changes, they are to keep the functionality of the
old log entry type intact during the change. If I had ported them over raw, they
would have looked awful. I cleaned up their behavior during the rewrite.

As for cleaning, you always clean the headers of spurious headers as soon as
possible. That's just being ordinarily responsible as a developer.

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
File src/plugin/ActiveQueue.h (right):

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:25: * A synchronized FIFO queue with thread notice on
receipt.
On 2015/10/01 15:50:06, sergei wrote:
> So, why is it called MessageQueue?

Because that's what it is, a message queue.

> It seems better to name is
> 'SynchronizedDeque'.

All message queues are synchronized. That's an entailment of what that phrase
ordinarily means. 

> Pay attention that I have used `Deque` because soon or late
> it will be deque, it uses deque internally and I've proposed `push_front` in
the
> comments.

It is contrary to every good practice of abstraction to name a class after its
underlying data structure. The whole point of abstraction to hide the
implementation details. It doesn't matter that it's implemented with
'std::deque'. Previous to using that implementation, I had it using a linked
list. Both underlying containers work.

Naming it as a double-ended queue is in fact _contrary_ to the intent of the
abstraction. Insofar as a user of this queue knows, all it has is an input.
What's put in "magically" disappears somewhere in the mist. There is, on
purpose, no corresponding member function to 'Insert'.

> As well as I don't like the exposing of methods which deal only with
> synchronization primitives.

Well, I'll certainly believe that you don't understand why it's that way. If you
don't expose anything that's under synchronization control of this class, you
need a second semaphore in the consumer class. That's just wasteful. You only
need one semaphore. This message queue class "lends" its semaphore to the queue
consumer in a structured way. It works great.

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:42: MessageQueue() {} // = default
On 2015/10/01 15:50:06, sergei wrote:
> Why do we need `= default`? I would remove it.

It's for when we have C++11 available. The string "// = default" provides an
easy way to mark what needs to be upgraded in the future.

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:53: void Insert(const T& t)
On 2015/10/01 15:50:07, sergei wrote:
> Do you mind to rename it to `push_back` or `PushBack`?

I completely object. It's contrary to the abstraction being presented in the
class. This is a queue, insofar as its users see, with one end, whose other one
fades into the distance. There's no front or back, there's only one visible end.

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:55: {
On 2015/10/01 15:50:06, sergei wrote:
> This additional scope is not needed here nor in the `Insert(T&&)` method.

Yep. I think this is a left-over artifact from development.

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:83: void Rouse(Function& f)
On 2015/10/01 15:50:06, sergei wrote:
> The reason we need such method is to set a value of `ActiveQueue:::running`.
We
> can achieve it with having `push_front` method here which looks more
appropriate
> as a member of `MessageQueue` class.

You get a race condition if you do it that way. The real reason you need 'Rouse'
is to ensure that the queue drains fully on destruction.

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:102: bool Remove(Function& f)
On 2015/10/01 15:50:06, sergei wrote:
> Why not to call it `pop_front` or `try_pop_front(... timeout)` and make this
> method waiting for the element in the queue?

As I've said before, this is not any kind of general-purpose double-ended queue.
Naming the front and back is contrary to the design intention of this class.

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:112: Placeholder<T> x;
On 2015/10/01 15:50:06, sergei wrote:
> I see only one reason

You see only one reason, yet there are other reasons.

> to use a class `Placeholder` which is an optimization of
> storing the object on the stack.

Nope. The reason the placeholder here is to enable the queue to be instantiated
with classes that have no copy constructors. This is not a theoretic
possibility. 'std::unique_ptr' is not copyable, nor is any class containing a
member of that type.

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:131: bool WaitIf(Predicate& p)
On 2015/10/01 15:50:06, sergei wrote:
> It's still `MessageQueue` class, how is this method related with a queue
nature
> of it? It seems better to have a method which waits until any element is added
> into the queue.
> Actually such waiting can be done in `Remove` method.

You get a race condition if you do it that way. Trust me. Fixed that already.

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:158: WAIT_FOR_RETURN_TYPE WaitFor(const
std::chrono::duration<R,P>& relativeDuration)
On 2015/10/01 15:50:06, sergei wrote:
> Why do we need this method? It seems to be not used and it's definitely has no
> relation to the queue nature.

It's used in the unit tests. Since you didn't comment on them, I'm guessing you
didn't read them. The message queue class is tested independently and separately
from the active queue.

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:180: class ActiveQueue
On 2015/10/01 15:50:07, sergei wrote:
> I would like to get rid of template parameters T and F and make it working
only
> with `std::function<void()>` as T. 

And I would like to keep them. You're suggesting needlessly narrowing the
generality of the code, reducing its maintainability by prematurely settling on
implementation details. I have no desire to go back to a less mature period of
software development.

> Having that the code will be more reliable

Such a change will have no impact on reliability.

> and easier to understand.

You mean "harder to understand". It's my experience that functional code in C++
is much clearer to read (and use) when you don't specify the particular function
classes.

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:191: bool running;
On 2015/10/01 15:50:07, sergei wrote:
> Hint: if we access it only from the working thread then we don't need any
> protection.

Again: race conditions. And there's no extra protection used in 'ActiveQueue';
it uses the semaphore that's in its 'MessageQueue' member.

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:196: F& processor;
On 2015/10/01 15:50:07, sergei wrote:
> In general it's a bad idea to use reference member type, especially in so
> generic class. Even more, in C++ it's usually assumed that a functor can be
> copied any number of times, 

You did notice that this class contains a thread member, and so is not copyable,
right?

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:225: while (
On 2015/10/01 15:50:06, sergei wrote:
> It means that it will run until the queue is not empty even if `running` is
> already false. Is it intended?

Of course it's intended. The whole point here is that whatever you insert into
an 'ActiveQueue' is always consumed, no matter what. If an 'ActiveQueue' is
destroyed when it still has messages in it, it consumes those messages before
the destructor returns.

Remember that I built this queue to log debug and trace messages to disk. The
behavior you are suggesting is that we simply drop some of these messages if
they show up inconveniently late. That is _exactly_ the opposite what we need
for that purpose. The previous logging system could, in fact, drop messages at
termination.

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/Placehol...
File src/plugin/Placeholder.h (right):

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/Placehol...
src/plugin/Placeholder.h:35: class Placeholder
On 2015/10/01 15:50:07, sergei wrote:
> The destructor is missing.

What would such a destructor do?

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/PluginDe...
File src/plugin/PluginDebug.h (right):

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/PluginDe...
src/plugin/PluginDebug.h:132: static pointer queueMasterReference;
On 2015/10/01 15:50:07, sergei wrote:
> Why do we need more static stuff?

In the present case, we want a single log facility for the whole DLL. Read the
comment above on why this isn't a simple static declaration, which would indeed
be simpler.

> We have already singletons for example
> CAdblockPlusClient and CPluginSettings, why not to combine them all in one
> singleton?

How would moving things out of their own scopes and creating a new _de facto_
global scope provide any benefit whatsoever? There's a reason for the consensus
that global variables are bad. When you've got separable responsibilities, you
put them in separate objects.

In fact, the lifetimes of these other objects do _not_ coincide with that of the
log queue. The reason is in the comment above, because you can't statically
initialize a thread in a DLL. Even if combining everything were a good idea
(which it's not), you'd need to seriously rework the initialization of these
other classes.

[Eric, 2015-10-09 16:13:05]

Note that this review depends upon a previous one:
    https://codereview.adblockplus.org/5187613258416128/
This review was created 7 months ago. I posted the most recent substantive
revision (i.e. not just a rebase) a week shy of 5 months ago.

[Eric, 2015-11-26 13:15:34]

New patch set is rebase only. It follows the rebase of its dependency
https://codereview.adblockplus.org/5187613258416128/

[Eric, 2016-01-05 14:50:51]

Patch set 3 is a rebase to current tip. It's dependency was just committed, so
this review is completely ripe.

This review is now more than four months old, having received scant attention
before. Its predecessor took just shy of ten months to get committed. I hope
this one can be done in only half that time, in only five months.

[sergei, 2016-01-26 23:28:43]

On 2016/01/05 14:50:51, Eric wrote:
> Patch set 3 is a rebase to current tip. It's dependency was just committed, so
> this review is completely ripe.
> 
> This review is now more than four months old, having received scant attention
> before. Its predecessor took just shy of ten months to get committed. I hope
> this one can be done in only half that time, in only five months.

I disagree with the implementation of ActiveQueue. I recommend to firstly get
acquainted with book "C++ Concurrency in Action: Practical Multithreading" and
then explain the reason we need something which is so different from known to
most of C++ developers. It's similar to using of std and ATL, many people are
familiar and experienced with it, the maintainability and may be even the
quality of the code with yet one very uncommon implementation of common class is
lower.

I'm waiting for the opinion of others regarding ActiveQueue and splitting it
into several commits.

[Oleksandr, 2016-01-28 10:15:35]

> I disagree with the implementation of ActiveQueue. 

Yes, I disagree with the implementation of ActiveQueue as well. It seems to me
that just using something like concurrency::concurrent_queue would be better,
for example https://msdn.microsoft.com/en-us/library/ee355358(v=vs.120).aspx. I
think the Placeholder class can be dropped, as well.

Also, we have had a similar pattern in the installer where we were searching the
process by its name, and it has proven to be hard to maintain. So I disagree
with the whole idea behind this patch set, I think it is over engineering and we
can afford to be far less generic (and more readable) since this code is only
used for debugging and unlikely to be used anywhere else anyway. 

FYI I have commented in both patchsets. In the one where there was a discussion
already and in the latest one.

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
File src/plugin/ActiveQueue.h (right):

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:42: MessageQueue() {} // = default
On 2015/10/08 21:05:40, Eric wrote:
> On 2015/10/01 15:50:06, sergei wrote:
> > Why do we need `= default`? I would remove it.
> 
> It's for when we have C++11 available. The string "// = default" provides an
> easy way to mark what needs to be upgraded in the future.

We have quite few // = default comments in the code already. I don't think
there's much sense to reverse those now.

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:53: void Insert(const T& t)
On 2015/10/08 21:05:39, Eric wrote:
> On 2015/10/01 15:50:07, sergei wrote:
> > Do you mind to rename it to `push_back` or `PushBack`?
> 
> I completely object. It's contrary to the abstraction being presented in the
> class. This is a queue, insofar as its users see, with one end, whose other
one
> fades into the distance. There's no front or back, there's only one visible
end.

I wouldn't mind Insert as well. I think we should try to abstract out the
implementation.

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:131: bool WaitIf(Predicate& p)
On 2015/10/08 21:05:39, Eric wrote:
> On 2015/10/01 15:50:06, sergei wrote:
> > It's still `MessageQueue` class, how is this method related with a queue
> nature
> > of it? It seems better to have a method which waits until any element is
added
> > into the queue.
> > Actually such waiting can be done in `Remove` method.
> 
> You get a race condition if you do it that way. Trust me. Fixed that already.

I agree with Sergei, this method is very awkward.

https://codereview.adblockplus.org/29323611/diff/29333202/adblockplus.gyp
File adblockplus.gyp (right):

https://codereview.adblockplus.org/29323611/diff/29333202/adblockplus.gyp#new...
adblockplus.gyp:307: #'msbuild_toolset': 'v140',
Nit: Is this some test code commented out? Looks confusing here, IMO.

https://codereview.adblockplus.org/29323611/diff/29333202/src/plugin/ActiveQu...
File src/plugin/ActiveQueue.h (right):

https://codereview.adblockplus.org/29323611/diff/29333202/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:3: * Copyright (C) 2006-2015 Eyeo GmbH
Nit: Here and further sadly we have already updated the header to the year 2016

https://codereview.adblockplus.org/29323611/diff/29333202/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:31: *   Functor type of object to receive notice on
Insert().
Nit: I guess this parameter was removed at some point?

https://codereview.adblockplus.org/29323611/diff/29333202/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:51: * Insert an element, copy constructor version.
Nit: this is not a constructor. I think "lvalue version" and "rvalue version"
explains it better. Or "copy semantics" and "move semantics".

https://codereview.adblockplus.org/29323611/diff/29333202/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:55: {
Just to reiterate Sergei's point - the extra scope is not needed here.

https://codereview.adblockplus.org/29323611/diff/29333202/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:93: * If so, remove the next element from the consumer
end of the queue and pass it to the argument function
Nit: I think there are too many comments in this function. The next comment
merely describes the workflow. We should aim to make the code readable with
minimal comments, and I think these are redundant. In any case it would be good
to at least have row limit of 80 chars.

[Eric, 2016-02-03 18:59:15]

On 2016/01/26 23:28:43, sergei wrote:
> I disagree with the implementation of ActiveQueue.

You haven't identified any defect with it. Given your previous comments on it, I
think you don't understand even what it's supposed to be doing, which would
certainly interfere with your ability to understand the implementation. If you
find a defect, let me know. 

> explain the reason we need something which is so different from known to
> most of C++ developers. 

"So different"? It's just an active object. It's a well known technique.

Here's an article by Herb Sutter on the technique. I take it you know who Herb
Sutter is. He's not exactly a fringe character.
    Prefer Using Active Objects Instead of Naked Threads
   
http://www.drdobbs.com/parallel/prefer-using-active-objects-instead-of-n/2257...

On 2016/01/28 10:15:35, Oleksandr wrote:
> Yes, I disagree with the implementation of ActiveQueue as well. It seems to me
> that just using something like concurrency::concurrent_queue would be better,

There's no independent thread of control in that class; that's just its most
glaring deficiency. It's designed for polling rather than using notification.

In retrospect, I made a mistake to call 'ActiveQueue' a queue in the first
place. The class is a combination of (1) a queue and (2) an active consumer.
Because the consumer is part of the class, only one end of the queue remains
visible. I'm certainly open to renaming the class (and its source file).

> I think the Placeholder class can be dropped, as well.

Perhaps. I couldn't figure out a way of moving instance of a class with
no-copy/only-move semantics through the queue easily. There's an interaction
with standard containers that's where the difficulty is. Once we get to a point
where you all know what the goal is here, we can come back to this point.

> Also, we have had a similar pattern in the installer where we were searching
the
> process by its name, and it has proven to be hard to maintain.

About the only similarity is that both use function objects. If you see some
other similarity, let me know.

Frankly, that code in the installer succeeded exactly how I wanted to in
maintenance. We needed to change a selection criterion, and that was already
isolated to a function object, so that was the only code that needed changing.
It looked like a win to me.

> since this code is only
> used for debugging and unlikely to be used anywhere else anyway. 

1) Debugging is important. This was the code I used to verify that IE does not
always called the BHO destructor when it terminates. That was only possible
because the logging facility had an independent flow of control. In addition,
the current log threading is horribly broken. It's associating a thread with
each instance of 'CPluginTab', which is just wrong and hinders rewrites to
improve the code structure.

2-a) We have an existing problem where the engine is crashing, apparently
because it's being overloaded with too many simultaneous somethings. One of the
approaches to address this would be throttle engine requests through a queue.
The 'ActiveQueue' class would work fine to run a scheduler thread to manage that
queue independent of the worker threads. I'm not sure if this is necessary yet;
we need to get through the v8 upgrade to see if this problem remains the same.

2-b) Detached initialization is presently done in a single function, but it
would be better (over time) to split it up into properly scoped pieces of
initialization. The current detached initialization class does not reference
'ActiveQueue', but it could usefully use it instead of its own raw thread
instance.

I'll address code comments and get a new patch set up later today or tomorrow.

[Eric, 2016-02-04 21:01:45]

Patch set 4 addresses Oleksandr's comments. It doesn't change the gyp file,
pending discussion about whether to keep a separate test project for
ActiveQueue.

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
File src/plugin/ActiveQueue.h (right):

https://codereview.adblockplus.org/29323611/diff/29324420/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:131: bool WaitIf(Predicate& p)
On 2016/01/28 10:15:32, Oleksandr wrote:
> I agree with Sergei, this method is very awkward.

What's awkward about it? Testing a predicate under lock before waiting is a
standard way of avoiding race conditions in situations like these. It might seem
to read better if you tested outside the lock (in the caller), but that's
defective code.

https://codereview.adblockplus.org/29323611/diff/29333202/adblockplus.gyp
File adblockplus.gyp (right):

https://codereview.adblockplus.org/29323611/diff/29333202/adblockplus.gyp#new...
adblockplus.gyp:307: #'msbuild_toolset': 'v140',
On 2016/01/28 10:15:33, Oleksandr wrote:
> Nit: Is this some test code commented out? Looks confusing here, IMO.

Not so much test code as needed during development. Getting a move-only class to
work right meant temporarily upgrading the compiler to support "= delete" on
copy constructors.

Perhaps more importantly, I'm not sure we need to retain a second unit test
project here. I needed it so that I could use a different compiler (which was,
for reasons I've forgotten, problematic on the first unit test project). It does
have some utility in segregating out the thread-heavy unit tests from the rest,
since when these unit tests crash, they crash hard.

https://codereview.adblockplus.org/29323611/diff/29333202/src/plugin/ActiveQu...
File src/plugin/ActiveQueue.h (right):

https://codereview.adblockplus.org/29323611/diff/29333202/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:3: * Copyright (C) 2006-2015 Eyeo GmbH
On 2016/01/28 10:15:34, Oleksandr wrote:
> Nit: Here and further sadly we have already updated the header to the year
2016

Done.

https://codereview.adblockplus.org/29323611/diff/29333202/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:31: *   Functor type of object to receive notice on
Insert().
On 2016/01/28 10:15:34, Oleksandr wrote:
> Nit: I guess this parameter was removed at some point?

Yes. As I recall, I originally only had one class. I split it into two to
improve testability. This is a leftover.

Fixed.

https://codereview.adblockplus.org/29323611/diff/29333202/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:51: * Insert an element, copy constructor version.
On 2016/01/28 10:15:34, Oleksandr wrote:
> Nit: this is not a constructor. I think "lvalue version" and "rvalue version"
> explains it better. Or "copy semantics" and "move semantics".

OK. I'll use "copy semantics".

https://codereview.adblockplus.org/29323611/diff/29333202/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:55: {
On 2016/01/28 10:15:35, Oleksandr wrote:
> Just to reiterate Sergei's point - the extra scope is not needed here.

Done.

https://codereview.adblockplus.org/29323611/diff/29333202/src/plugin/ActiveQu...
src/plugin/ActiveQueue.h:93: * If so, remove the next element from the consumer
end of the queue and pass it to the argument function
Reworded.

[Eric, 2016-07-27 20:31:28]

Renamed description to add #1234. The old threading code for log writing is now
the only remaining use of CString in the code.