Issue 29539858: Issue 5506 - Make the notification test work. (Closed)
DescriptionIssue 5506 - Make the notification test work.
Patch Set 1 #
Total comments: 1
Patch Set 2 : This version work using a fake "ManualTimer" #
Total comments: 16
Patch Set 3 : Address review comments. #
Total comments: 4
Patch Set 4 : Updated with the feedback #Patch Set 5 : Removed dead code from previous patch #MessagesTotal messages: 12
not ready for review yet, the test segfault.
segfault occurs because JsEngine::ScheduleTimer() is being called as Platform::~Platform() is in progress in the other thread and it already has reset the timer unique_ptr<> with ~DefaultTimer() joining on the thread. From there jsEngine->platform.GetTimer() gets called and that returns a reference to nullptr....
https://codereview.adblockplus.org/29539858/diff/29539859/test/Notification.cpp File test/Notification.cpp (right): https://codereview.adblockplus.org/29539858/diff/29539859/test/Notification.c... test/Notification.cpp:107: platformParams.timer.reset(new AdblockPlus::DefaultTimer()); I would recommend to not use threads here and wait so long. There could be a mock timer and we could manually call a corresponding timer callback from the test body in order to simulate it. We could basically call all timer callbacks with 1 minute timeout, it seems the easiest way, though hacky.
ideally the cleanup phase should be safe, and it isn't. But I'll try to solve the timer issue differently.
current patch work, and it uses a proper synchronization to check the callback was called.
On 2017/09/11 21:16:31, hub wrote: > segfault occurs because JsEngine::ScheduleTimer() is being called as > Platform::~Platform() is in progress in the other thread and it already has > reset the timer unique_ptr<> with ~DefaultTimer() joining on the thread. > > From there jsEngine->platform.GetTimer() gets called and that returns a > reference to nullptr.... Is it the issue https://issues.adblockplus.org/ticket/5118? https://codereview.adblockplus.org/29539858/diff/29541886/test/Notification.cpp File test/Notification.cpp (right): https://codereview.adblockplus.org/29539858/diff/29541886/test/Notification.c... test/Notification.cpp:21: #include "../src/DefaultTimer.h" It seems these headers are not required. https://codereview.adblockplus.org/29539858/diff/29541886/test/Notification.c... test/Notification.cpp:91: AdblockPlus::Sync sync; It seems sync is not needed. https://codereview.adblockplus.org/29539858/diff/29541886/test/Notification.c... test/Notification.cpp:106: platformParams.logSystem.reset(new AdblockPlus::DefaultLogSystem()); What about using of ThrowingLogSystem as earlier? https://codereview.adblockplus.org/29539858/diff/29541886/test/Notification.c... test/Notification.cpp:107: platformParams.timer.reset(new ManualTimer()); I would recommend to use DelayedTimer and not create ManualTimer. https://codereview.adblockplus.org/29539858/diff/29541886/test/Notification.c... test/Notification.cpp:112: auto& filterEngine = platform->GetFilterEngine(); I think here is a dead lock, it should be as above CreateFilterEngine(*fileSystem, *platform); https://codereview.adblockplus.org/29539858/diff/29541886/test/Notification.c... test/Notification.cpp:133: static_cast<ManualTimer&>(platform->GetTimer()).runOne(); I still think that it would be more reliable to fire timers with the 1 minute timeout instead of a first one.
On 2017/09/12 09:22:48, sergei wrote: > On 2017/09/11 21:16:31, hub wrote: > > segfault occurs because JsEngine::ScheduleTimer() is being called as > > Platform::~Platform() is in progress in the other thread and it already has > > reset the timer unique_ptr<> with ~DefaultTimer() joining on the thread. > > > > From there jsEngine->platform.GetTimer() gets called and that returns a > > reference to nullptr.... > > Is it the issue https://issues.adblockplus.org/ticket/5118 Sounds like it, indeed.
reworked following review. https://codereview.adblockplus.org/29539858/diff/29541886/test/Notification.cpp File test/Notification.cpp (right): https://codereview.adblockplus.org/29539858/diff/29541886/test/Notification.c... test/Notification.cpp:21: #include "../src/DefaultTimer.h" On 2017/09/12 09:22:48, sergei wrote: > It seems these headers are not required. Done. https://codereview.adblockplus.org/29539858/diff/29541886/test/Notification.c... test/Notification.cpp:91: AdblockPlus::Sync sync; On 2017/09/12 09:22:48, sergei wrote: > It seems sync is not needed. yes it is. Line 112 and line 135 https://codereview.adblockplus.org/29539858/diff/29541886/test/Notification.c... test/Notification.cpp:106: platformParams.logSystem.reset(new AdblockPlus::DefaultLogSystem()); On 2017/09/12 09:22:48, sergei wrote: > What about using of ThrowingLogSystem as earlier? Done. https://codereview.adblockplus.org/29539858/diff/29541886/test/Notification.c... test/Notification.cpp:107: platformParams.timer.reset(new ManualTimer()); On 2017/09/12 09:22:48, sergei wrote: > I would recommend to use DelayedTimer and not create ManualTimer. DelayedTimer doesn't do what I want. The idea of ManualTimer is to ignore the delay and just run them. https://codereview.adblockplus.org/29539858/diff/29541886/test/Notification.c... test/Notification.cpp:112: auto& filterEngine = platform->GetFilterEngine(); On 2017/09/12 09:22:48, sergei wrote: > I think here is a dead lock, it should be as above > CreateFilterEngine(*fileSystem, *platform); Done. https://codereview.adblockplus.org/29539858/diff/29541886/test/Notification.c... test/Notification.cpp:133: static_cast<ManualTimer&>(platform->GetTimer()).runOne(); On 2017/09/12 09:22:48, sergei wrote: > I still think that it would be more reliable to fire timers with the 1 minute > timeout instead of a first one. I changed it to a runPending() which will run all pending timers.
https://codereview.adblockplus.org/29539858/diff/29541886/test/Notification.cpp File test/Notification.cpp (right): https://codereview.adblockplus.org/29539858/diff/29541886/test/Notification.c... test/Notification.cpp:91: AdblockPlus::Sync sync; On 2017/09/12 12:59:09, hub wrote: > On 2017/09/12 09:22:48, sergei wrote: > > It seems sync is not needed. > > yes it is. Line 112 and line 135 There are no other threads, so sync is not needed, it can be `bool isNotificationCallbackCalled`. BTW, it's removed in another codereview. In case we need such functionality we should rather use std::promise/std::future. https://codereview.adblockplus.org/29539858/diff/29541886/test/Notification.c... test/Notification.cpp:107: platformParams.timer.reset(new ManualTimer()); On 2017/09/12 12:59:09, hub wrote: > On 2017/09/12 09:22:48, sergei wrote: > > I would recommend to use DelayedTimer and not create ManualTimer. > > DelayedTimer doesn't do what I want. The idea of ManualTimer is to ignore the > delay and just run them. I still think that DelayedTimer is better here because it basically already has the functionality of the pending tasks (should be a member of the fixture) and we should run tasks until our callback is executed (isNotificationCallbackCalled is set). So, the test body would be (NOT TESTED): auto& filterEngine = platform->GetFilterEngine(); auto ii = timerTasks->begin(); while(!isNotificationCallbackCalled && ii != timerTasks->end()) { ii->callback(); ii = timerTasks->erase(ii); filterEngine.ShowNextNotification(); } EXPECT_TRUE(isNotificationCallbackCalled); https://codereview.adblockplus.org/29539858/diff/29542589/test/Notification.cpp File test/Notification.cpp (right): https://codereview.adblockplus.org/29539858/diff/29542589/test/Notification.c... test/Notification.cpp:19: #include <AdblockPlus/DefaultLogSystem.h> These headers are still not required. https://codereview.adblockplus.org/29539858/diff/29542589/test/Notification.c... test/Notification.cpp:106: platformParams.logSystem.reset(new ThrowingLogSystem()); These two lines can be replaced by original `ThrowingPlatformCreationParameters platformParams`, not a big difference, though.
Updated patch. https://codereview.adblockplus.org/29539858/diff/29541886/test/Notification.cpp File test/Notification.cpp (right): https://codereview.adblockplus.org/29539858/diff/29541886/test/Notification.c... test/Notification.cpp:91: AdblockPlus::Sync sync; On 2017/09/12 13:34:09, sergei wrote: > On 2017/09/12 12:59:09, hub wrote: > > On 2017/09/12 09:22:48, sergei wrote: > > > It seems sync is not needed. > > > > yes it is. Line 112 and line 135 > > There are no other threads, so sync is not needed, it can be `bool > isNotificationCallbackCalled`. BTW, it's removed in another codereview. In case > we need such functionality we should rather use std::promise/std::future. Done. https://codereview.adblockplus.org/29539858/diff/29541886/test/Notification.c... test/Notification.cpp:107: platformParams.timer.reset(new ManualTimer()); On 2017/09/12 13:34:09, sergei wrote: > On 2017/09/12 12:59:09, hub wrote: > > On 2017/09/12 09:22:48, sergei wrote: > > > I would recommend to use DelayedTimer and not create ManualTimer. > > > > DelayedTimer doesn't do what I want. The idea of ManualTimer is to ignore the > > delay and just run them. > > I still think that DelayedTimer is better here because it basically already has > the functionality of the pending tasks (should be a member of the fixture) and > we should run tasks until our callback is executed (isNotificationCallbackCalled > is set). > So, the test body would be (NOT TESTED): > auto& filterEngine = platform->GetFilterEngine(); > auto ii = timerTasks->begin(); > while(!isNotificationCallbackCalled && ii != timerTasks->end()) > { > ii->callback(); > ii = timerTasks->erase(ii); > filterEngine.ShowNextNotification(); > } > EXPECT_TRUE(isNotificationCallbackCalled); I see. Done. https://codereview.adblockplus.org/29539858/diff/29542589/test/Notification.cpp File test/Notification.cpp (right): https://codereview.adblockplus.org/29539858/diff/29542589/test/Notification.c... test/Notification.cpp:19: #include <AdblockPlus/DefaultLogSystem.h> On 2017/09/12 13:34:09, sergei wrote: > These headers are still not required. Done. https://codereview.adblockplus.org/29539858/diff/29542589/test/Notification.c... test/Notification.cpp:106: platformParams.logSystem.reset(new ThrowingLogSystem()); On 2017/09/12 13:34:09, sergei wrote: > These two lines can be replaced by original `ThrowingPlatformCreationParameters > platformParams`, not a big difference, though. Done.
LGTM 
|
