Issue 4944 - Make creation of FilterEngine asynchronous.

[Eric, 2017-02-28 17:04:50]

I didn't read the rest of the code once I looked at the signature of the async
factory. When that's good, I'll look at the rest of the code.

https://codereview.adblockplus.org/29377064/diff/29377560/include/AdblockPlus...
File include/AdblockPlus/FilterEngine.h (right):

https://codereview.adblockplus.org/29377064/diff/29377560/include/AdblockPlus...
include/AdblockPlus/FilterEngine.h:241: static void CreateAsync(const
JsEnginePtr& jsEngine,
(1) C++ has <future>.

(2) This is new code. This is not a rewrite of legacy code.

(3) An asynchronous factory should be presented in the standard C++ way, which
is with std::promise.

Among other deficiencies in a design pattern that uses a callback, there's no
good way to deal with a constructor exception. std::future handles that case
seamlessly.

I seem to remember there are some issues with <future> in VS 2012. I don't know
the details, nor do I know whether they'll cause a problem with this code. All
the more reason to concentrate on getting our tools updated, even if it means
delaying some desired features. The friction and drag from not having current
tools just keeps accumulating.

[sergei, 2017-02-28 22:18:27]

https://codereview.adblockplus.org/29377064/diff/29377560/include/AdblockPlus...
File include/AdblockPlus/FilterEngine.h (right):

https://codereview.adblockplus.org/29377064/diff/29377560/include/AdblockPlus...
include/AdblockPlus/FilterEngine.h:241: static void CreateAsync(const
JsEnginePtr& jsEngine,
On 2017/02/28 17:04:50, Eric wrote:
> (1) C++ has <future>.
> 
> (2) This is new code. This is not a rewrite of legacy code.
> 
> (3) An asynchronous factory should be presented in the standard C++ way, which
> is with std::promise.
> 
> Among other deficiencies in a design pattern that uses a callback, there's no
> good way to deal with a constructor exception. std::future handles that case
> seamlessly.
> 
> I seem to remember there are some issues with <future> in VS 2012. I don't
know
> the details, nor do I know whether they'll cause a problem with this code. All
> the more reason to concentrate on getting our tools updated, even if it means
> delaying some desired features. The friction and drag from not having current
> tools just keeps accumulating.

Sorry, but what is the aim of this message? We currently bound to MSVS2012 and
std::promise and std::future are completely broken here. Of course they should
be used but we cannot do it. JIC, I have not deliberately brought the
implementation of Sync which is aware about exceptions. I hope that anyway it
will go away in favor of std::promise and std::future after updating of v8 when
we can switch to toolset v140.

[Eric, 2017-02-28 23:06:12]

On 2017/02/28 22:18:27, sergei wrote:
> Sorry, but what is the aim of this message? 

I am pointing out that by continuing to drag our heels in getting our toolset
updated, we are shooting ourselves in the foot over and over again.

I am saying that our priorities are wrong if we are continuing to need half-wit
implementations of primitives that are already in the standard library. I've had
to do it, and I really dislike it.

I am recommending that we _not_ push this change until _after_ we can use
<future>. I don't think this has any downside, because we don't need this at
present, since the change that needs it can be done entirely with JS code.)

> I hope that anyway it
> will go away in favor of std::promise and std::future after updating of v8
when
> we can switch to toolset v140.

I would greatly recommend that we stop writing fresh-legacy code and do it right
from the outset. If you feel that you don't have resources to get the change
done quickly, please ask our management to prioritize the attention of whoever
it is that needs to be pay attention to this so that we can move forward.

[sergei, 2017-03-01 07:19:13]

BTW, std::future does not have continuation "then" yet, so I would like to still
keep callback later because not everyone wants to poll or be blocked. So, most
likely internal implementation will be affected and a new method returning
std::future will be added.

[Eric, 2017-03-01 15:19:38]

On 2017/03/01 07:19:13, sergei wrote:
> BTW, std::future does not have continuation "then" yet

If you insist on coding like JavaScript, you can implement "then" in several
different ways.
   
http://stackoverflow.com/questions/14489935/implementing-futurethen-equivalen...

> so I would like to still
> keep callback later because not everyone wants to poll or be blocked.

Why do you think that somehow this is impossible? The interface already supports
a call that won't block.

    `f.wait_for(chrono::seconds(0))`

I don't know why this idiom didn't get a separate API function.

[sergei, 2017-03-01 15:22:52]

On 2017/03/01 15:19:38, Eric wrote:
> On 2017/03/01 07:19:13, sergei wrote:
> > BTW, std::future does not have continuation "then" yet
> 
> If you insist on coding like JavaScript, you can implement "then" in several
> different ways.
>    
>
http://stackoverflow.com/questions/14489935/implementing-futurethen-equivalen...
> 
> > so I would like to still
> > keep callback later because not everyone wants to poll or be blocked.
> 
> Why do you think that somehow this is impossible? The interface already
supports
> a call that won't block.
> 
>     `f.wait_for(chrono::seconds(0))`
> 
> I don't know why this idiom didn't get a separate API function.
`f.wait_for(chrono::seconds(0))` is good for polling but why will someone want
to do it this way?

[Eric, 2017-03-01 15:44:20]

The API you're proposing, the one with a callback, enforces a single way of
using the factory. Returning a future allows multiple ways of using it. If you
want to allow someone to receive a callback notice, you can do that with some
code that takes a future as an argument.

Why would good library design coerce its users into using it in only one way?

[sergei, 2017-03-16 08:56:27]

On 2017/03/01 15:44:20, Eric wrote:
> The API you're proposing, the one with a callback, enforces a single way of
> using the factory. Returning a future allows multiple ways of using it. If you
> want to allow someone to receive a callback notice, you can do that with some
> code that takes a future as an argument.
> 
> Why would good library design coerce its users into using it in only one way?
I think that interface with only future is less flexible than with callback.

[Oleksandr, 2017-03-16 15:05:25]

LGTM with the nit addressed.

https://codereview.adblockplus.org/29377064/diff/29377560/include/AdblockPlus...
File include/AdblockPlus/FilterEngine.h (right):

https://codereview.adblockplus.org/29377064/diff/29377560/include/AdblockPlus...
include/AdblockPlus/FilterEngine.h:219: struct CreateParameters
Nit: How about CreationParameters. One might assume this is a function with a
name as you have it.

https://codereview.adblockplus.org/29377064/diff/29377560/include/AdblockPlus...
include/AdblockPlus/FilterEngine.h:241: static void CreateAsync(const
JsEnginePtr& jsEngine,
On 2017/02/28 22:18:27, sergei wrote:
> On 2017/02/28 17:04:50, Eric wrote:
> > (1) C++ has <future>.
> > 
> > (2) This is new code. This is not a rewrite of legacy code.
> > 
> > (3) An asynchronous factory should be presented in the standard C++ way,
which
> > is with std::promise.
> > 
> > Among other deficiencies in a design pattern that uses a callback, there's
no
> > good way to deal with a constructor exception. std::future handles that case
> > seamlessly.
> > 
> > I seem to remember there are some issues with <future> in VS 2012. I don't
> know
> > the details, nor do I know whether they'll cause a problem with this code.
All
> > the more reason to concentrate on getting our tools updated, even if it
means
> > delaying some desired features. The friction and drag from not having
current
> > tools just keeps accumulating.
> 
> Sorry, but what is the aim of this message? We currently bound to MSVS2012 and
> std::promise and std::future are completely broken here. Of course they should
> be used but we cannot do it. JIC, I have not deliberately brought the
> implementation of Sync which is aware about exceptions. I hope that anyway it
> will go away in favor of std::promise and std::future after updating of v8
when
> we can switch to toolset v140.

It does not look like we'll be able to move to a newer Visual Studio in the
proposed time frame, unfortunately. So discussion about std::future doesn't make
much sense at this point.

[sergei, 2017-03-16 15:50:03]

https://codereview.adblockplus.org/29377064/diff/29377560/include/AdblockPlus...
File include/AdblockPlus/FilterEngine.h (right):

https://codereview.adblockplus.org/29377064/diff/29377560/include/AdblockPlus...
include/AdblockPlus/FilterEngine.h:219: struct CreateParameters
On 2017/03/16 15:05:25, Oleksandr wrote:
> Nit: How about CreationParameters. One might assume this is a function with a
> name as you have it.

Agree, done.

[sergei, 2017-03-17 14:39:39]

I'm going to push it today.