Issue 5118 - Lock the platform interfaces before use

Issue 5118 - Lock the platform interfaces before use

[hub, 2017-09-13 17:34:48]

I forgot LogSystem.

[sergei, 2017-09-13 19:20:53]

Although I'm not sure about the approach in general, despite it was me, who
described it in the issue :), I think it's fine, the only stopper is in the
destructor.

https://codereview.adblockplus.org/29543810/diff/29543817/include/AdblockPlus...
File include/AdblockPlus/Platform.h (right):

https://codereview.adblockplus.org/29543810/diff/29543817/include/AdblockPlus...
include/AdblockPlus/Platform.h:129: std::recursive_mutex interfacesMutex;
What do you think about making the methods WithSomeInterface virtual, moving the
mutex into DefaultPlatform and overriding the corresponding methods there which
are using the mutex? I think about it because we basically "should pay only for
what we use", if the implementation of the interfaces does not imply
multi-threading then then this mutex is not required. Though, it seems a small
optimization and there is still `modulesMutex`, which should not be touched in
this codereview.

https://codereview.adblockplus.org/29543810/diff/29543817/src/Platform.cpp
File src/Platform.cpp (right):

https://codereview.adblockplus.org/29543810/diff/29543817/src/Platform.cpp#ne...
src/Platform.cpp:57: timer.reset();
I'm afraid here is a dead lock:
Let's say `interfacesMutex` has been acquired here and the current thread is on
the line 56, meanwhile the timer callback is being fired and gets locked on the
same mutex, now our thread reaches the 57th line and destroys the timer which
will wait for the finishing of its thread what will never happen because the
timer's thread is waiting for the mutex.
I think it should be rather
{
  LogSystemPtr tmpLogSystem;
  ...
  {
    std::lock_guard<std::recursive_mutex> lock(interfacesMutex);
    tmpLogSystem = move(logSystem);
    ...
  }
  // here we can safely destroy the interfaces because the mutex is unlocked and
they can wait for their threads but any new scheduled tasks (functions passed
into WithSomeInterface) will be ignored.
}

[hub, 2017-09-13 19:50:50]

https://codereview.adblockplus.org/29543810/diff/29543817/include/AdblockPlus...
File include/AdblockPlus/Platform.h (right):

https://codereview.adblockplus.org/29543810/diff/29543817/include/AdblockPlus...
include/AdblockPlus/Platform.h:129: std::recursive_mutex interfacesMutex;
On 2017/09/13 19:20:53, sergei wrote:
> What do you think about making the methods WithSomeInterface virtual, moving
the
> mutex into DefaultPlatform and overriding the corresponding methods there
which
> are using the mutex? I think about it because we basically "should pay only
for
> what we use", if the implementation of the interfaces does not imply
> multi-threading then then this mutex is not required. Though, it seems a small
> optimization and there is still `modulesMutex`, which should not be touched in
> this codereview.

I can do that.

https://codereview.adblockplus.org/29543810/diff/29543817/src/Platform.cpp
File src/Platform.cpp (right):

https://codereview.adblockplus.org/29543810/diff/29543817/src/Platform.cpp#ne...
src/Platform.cpp:57: timer.reset();
On 2017/09/13 19:20:53, sergei wrote:
> I'm afraid here is a dead lock:
> Let's say `interfacesMutex` has been acquired here and the current thread is
on
> the line 56, meanwhile the timer callback is being fired and gets locked on
the
> same mutex, now our thread reaches the 57th line and destroys the timer which
> will wait for the finishing of its thread what will never happen because the
> timer's thread is waiting for the mutex.
> I think it should be rather
> {
>   LogSystemPtr tmpLogSystem;
>   ...
>   {
>     std::lock_guard<std::recursive_mutex> lock(interfacesMutex);
>     tmpLogSystem = move(logSystem);
>     ...
>   }
>   // here we can safely destroy the interfaces because the mutex is unlocked
and
> they can wait for their threads but any new scheduled tasks (functions passed
> into WithSomeInterface) will be ignored.
> }

Good catch.

[sergei, 2017-09-13 20:06:38]

LGTM, however could you please still address this couple of rather decorative
things.

https://codereview.adblockplus.org/29543810/diff/29543824/src/Platform.cpp
File src/Platform.cpp (right):

https://codereview.adblockplus.org/29543810/diff/29543824/src/Platform.cpp#ne...
src/Platform.cpp:134: virtual void WithTimer(const WithTimerCallback&) override;
Could you please remove the `virtual` keyword here for overridden methods
because it's already clear that they are virtual from `override`.

https://codereview.adblockplus.org/29543810/diff/29543824/src/Platform.cpp#ne...
src/Platform.cpp:161: tmpWebRequest.reset();
I think this calls of reset are not required.

[hub, 2017-09-13 20:17:51]

https://codereview.adblockplus.org/29543810/diff/29543824/src/Platform.cpp
File src/Platform.cpp (right):

https://codereview.adblockplus.org/29543810/diff/29543824/src/Platform.cpp#ne...
src/Platform.cpp:134: virtual void WithTimer(const WithTimerCallback&) override;
On 2017/09/13 20:06:37, sergei wrote:
> Could you please remove the `virtual` keyword here for overridden methods
> because it's already clear that they are virtual from `override`.

Done.

https://codereview.adblockplus.org/29543810/diff/29543824/src/Platform.cpp#ne...
src/Platform.cpp:161: tmpWebRequest.reset();
On 2017/09/13 20:06:37, sergei wrote:
> I think this calls of reset are not required.

Done.

[sergei, 2017-09-13 20:26:04]

LGTM