Issue 5179 - Implement asynchronous executor with a controllable lifetime

[sergei, 2018-02-23 11:16:42]

https://codereview.adblockplus.org/29706560/diff/29706561/.travis.yml
File .travis.yml (right):

https://codereview.adblockplus.org/29706560/diff/29706561/.travis.yml#newcode44
.travis.yml:44: - travis_wait 30 make ${BUILD_ACTION}
LIBV8_LIB_DIR=../third_party/prebuilt-v8/
LIBV8_INCLUDE_DIR=third_party/prebuilt-v8/include
After numerous iterations of adjusting parameters perhaps it's not needed
already, but since the resources on travis-ci seem pretty dynamic (sometimes the
execution is really 5 times slower) I would like to keep it JIC.

https://codereview.adblockplus.org/29706560/diff/29706561/src/Platform.cpp
File src/Platform.cpp (right):

https://codereview.adblockplus.org/29706560/diff/29706561/src/Platform.cpp#ne...
src/Platform.cpp:120: class SharedAsyncExecutor {
It's rather a compatibility thing to simplify the updating of libadblockplus in
libadblockplus-android which will already cause a lot of work (because they will
have to figure out what to do with infinitely running operations). In
libadblockplus-android we are currently using
DefaultPlatformBuilder::GetDefaultAsyncExecutor() to obtain a scheduler
(std::function) which is used to safely run asynchronous tasks related to
FilterEngine. In general it's not a public API and under the question to be
changed later.

When DefaultPlatform is being destroyed we invalidate that asynchronous executor
and ensure that no any task is running there, so whatever scheduled by
libadblocklpus-android via the function defaultScheduler will be either able to
normally (without access violations) finish because FilterEngine is not
destroyed yet or ignored at the scheduling stage, what is also safe.

See how it's used with `defaultScheduler` and that it's "invalidated" in the
destructor.

https://codereview.adblockplus.org/29706560/diff/29706561/src/Platform.cpp#ne...
src/Platform.cpp:133: std::unique_ptr<AsyncExecutor> tmp;
JIC, it's needed to prevent dead locks. If `executor` were destroyed directly
under the `lock` then it could happen that one of the tasks of the `executor` is
waiting for the lock on mutex in order to dispatch a next task and the
`executor` is waiting for the finishing of that task and is not releasing the
mutex.

[hub, 2018-02-23 22:59:35]

https://codereview.adblockplus.org/29706560/diff/29706561/include/AdblockPlus...
File include/AdblockPlus/SynchronizedCollection.h (right):

https://codereview.adblockplus.org/29706560/diff/29706561/include/AdblockPlus...
include/AdblockPlus/SynchronizedCollection.h:61: * Extracts normally a firtst of
currently stored elements and returns it.
I think the comment should read "Extracts the first stored element and returns
it".

https://codereview.adblockplus.org/29706560/diff/29706561/src/Platform.cpp
File src/Platform.cpp (right):

https://codereview.adblockplus.org/29706560/diff/29706561/src/Platform.cpp#ne...
src/Platform.cpp:148: explicit DefaultPlatform(const
std::shared_ptr<SharedAsyncExecutor>& asyncExecutor, CreationParameters&&
creationParams)
Since there is `typedef std::shared_ptr<SharedAsyncExecutor> AsyncExecutorPtr`
above, why isn't AsyncExecutorPtr used here. (and below line 160) ?

[sergei, 2018-03-01 11:18:38]

https://codereview.adblockplus.org/29706560/diff/29706561/include/AdblockPlus...
File include/AdblockPlus/SynchronizedCollection.h (right):

https://codereview.adblockplus.org/29706560/diff/29706561/include/AdblockPlus...
include/AdblockPlus/SynchronizedCollection.h:61: * Extracts normally a firtst of
currently stored elements and returns it.
On 2018/02/23 22:59:35, hub wrote:
> I think the comment should read "Extracts the first stored element and returns
> it".

Yeah, perhaps it's better, done.

https://codereview.adblockplus.org/29706560/diff/29706561/src/Platform.cpp
File src/Platform.cpp (right):

https://codereview.adblockplus.org/29706560/diff/29706561/src/Platform.cpp#ne...
src/Platform.cpp:148: explicit DefaultPlatform(const
std::shared_ptr<SharedAsyncExecutor>& asyncExecutor, CreationParameters&&
creationParams)
On 2018/02/23 22:59:35, hub wrote:
> Since there is `typedef std::shared_ptr<SharedAsyncExecutor> AsyncExecutorPtr`
> above, why isn't AsyncExecutorPtr used here. (and below line 160) ?

Fixed, I somehow forgot about it ))

[hub, 2018-03-01 18:43:03]

LGTM