From 0e8ceb97a1d61be5bfe66d4281270f693c24f4aa Mon Sep 17 00:00:00 2001 From: Lev Dragunov Date: Wed, 11 Nov 2015 16:36:07 +0300 Subject: [PATCH 1/3] Routing session tests UB fix. --- .../routing_tests/routing_session_test.cpp | 34 +++++++++++++------ 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/routing/routing_tests/routing_session_test.cpp b/routing/routing_tests/routing_session_test.cpp index 62ae8c2d0e..1dd95aed98 100644 --- a/routing/routing_tests/routing_session_test.cpp +++ b/routing/routing_tests/routing_session_test.cpp @@ -8,8 +8,8 @@ #include "base/logging.hpp" +#include "std/atomic.hpp" #include "std/chrono.hpp" -#include "std/mutex.hpp" #include "std/string.hpp" #include "std/vector.hpp" @@ -42,7 +42,7 @@ public: }; static vector kTestRoute = {{0., 1.}, {0., 2.}, {0., 3.}, {0., 4.}}; -static auto kRouteBuildingMaxDuration = seconds(30); +static auto kTestMaxDuration = seconds(30); UNIT_TEST(TestRouteBuilding) { @@ -51,17 +51,21 @@ UNIT_TEST(TestRouteBuilding) vector routePoints = kTestRoute; Route masterRoute("dummy", routePoints.begin(), routePoints.end()); size_t counter = 0; - timed_mutex routeBuilded; - routeBuilded.lock(); + atomic routeBuilded(false); unique_ptr router = make_unique(masterRoute, DummyRouter::NoError, counter); session.SetRouter(move(router), nullptr); session.BuildRoute(kTestRoute.front(), kTestRoute.back(), [&routeBuilded](Route const &, IRouter::ResultCode) { - routeBuilded.unlock(); + routeBuilded = true; }, nullptr, 0); - TEST(routeBuilded.try_lock_for(kRouteBuildingMaxDuration), ()); + // Manual check of the routeBuilded mutex to avoid spurious results. + auto time = steady_clock::now() + kTestMaxDuration; + while (steady_clock::now() < time && !routeBuilded) + { + } + TEST(routeBuilded, ("Route was not built.")); TEST_EQUAL(counter, 1, ()); } @@ -73,18 +77,22 @@ UNIT_TEST(TestRouteRebuilding) vector routePoints = kTestRoute; Route masterRoute("dummy", routePoints.begin(), routePoints.end()); size_t counter = 0; - timed_mutex routeBuilded; + atomic routeBuilded(false); auto fn = [&routeBuilded](Route const &, IRouter::ResultCode) { - routeBuilded.unlock(); + routeBuilded = true; }; - routeBuilded.lock(); unique_ptr router = make_unique(masterRoute, DummyRouter::NoError, counter); session.SetRouter(move(router), nullptr); // Go along the route. session.BuildRoute(kTestRoute.front(), kTestRoute.back(), fn, nullptr, 0); - TEST(routeBuilded.try_lock_for(kRouteBuildingMaxDuration), ()); + // Manual check of the routeBuilded mutex to avoid spurious results. + auto time = steady_clock::now() + kTestMaxDuration; + while (steady_clock::now() < time && !routeBuilded) + { + } + TEST(routeBuilded, ("Route was not built.")); location::GpsInfo info; info.m_horizontalAccuracy = 0.01; @@ -103,8 +111,12 @@ UNIT_TEST(TestRouteRebuilding) // Rebuild route and go in opposite direction. So initiate a route rebuilding flag. counter = 0; + routeBuilded = false; session.BuildRoute(kTestRoute.front(), kTestRoute.back(), fn, nullptr, 0); - TEST(routeBuilded.try_lock_for(kRouteBuildingMaxDuration), ()); + while (steady_clock::now() < time && !routeBuilded) + { + } + TEST(routeBuilded, ("Route was not built.")); info.m_longitude = 0.; info.m_latitude = 1.; From 7ee36342d0f6a663051702a3837de5b6e5729ff0 Mon Sep 17 00:00:00 2001 From: Lev Dragunov Date: Wed, 11 Nov 2015 18:05:26 +0300 Subject: [PATCH 2/3] Rewrite routing session tests sync to condition variable. --- .../routing_tests/routing_session_test.cpp | 51 ++++++++++--------- 1 file changed, 28 insertions(+), 23 deletions(-) diff --git a/routing/routing_tests/routing_session_test.cpp b/routing/routing_tests/routing_session_test.cpp index 1dd95aed98..c5bd3aa258 100644 --- a/routing/routing_tests/routing_session_test.cpp +++ b/routing/routing_tests/routing_session_test.cpp @@ -8,8 +8,8 @@ #include "base/logging.hpp" -#include "std/atomic.hpp" #include "std/chrono.hpp" +#include "std/mutex.hpp" #include "std/string.hpp" #include "std/vector.hpp" @@ -42,7 +42,7 @@ public: }; static vector kTestRoute = {{0., 1.}, {0., 2.}, {0., 3.}, {0., 4.}}; -static auto kTestMaxDuration = seconds(30); +static auto kRouteBuildingMaxDuration = seconds(30); UNIT_TEST(TestRouteBuilding) { @@ -51,21 +51,24 @@ UNIT_TEST(TestRouteBuilding) vector routePoints = kTestRoute; Route masterRoute("dummy", routePoints.begin(), routePoints.end()); size_t counter = 0; - atomic routeBuilded(false); + bool routeBuilt(false); + mutex waitingMutex; + unique_lock lock(waitingMutex); + condition_variable cv; unique_ptr router = make_unique(masterRoute, DummyRouter::NoError, counter); session.SetRouter(move(router), nullptr); session.BuildRoute(kTestRoute.front(), kTestRoute.back(), - [&routeBuilded](Route const &, IRouter::ResultCode) + [&routeBuilt, &waitingMutex, &cv](Route const &, IRouter::ResultCode) { - routeBuilded = true; + lock_guard guard(waitingMutex); + routeBuilt = true; + cv.notify_one(); }, nullptr, 0); // Manual check of the routeBuilded mutex to avoid spurious results. - auto time = steady_clock::now() + kTestMaxDuration; - while (steady_clock::now() < time && !routeBuilded) - { - } - TEST(routeBuilded, ("Route was not built.")); + auto const time = steady_clock::now() + kRouteBuildingMaxDuration; + cv.wait_until(lock, time, [&routeBuilt, &time]{return routeBuilt || steady_clock::now() > time;}); + TEST(routeBuilt, ("Route was not built.")); TEST_EQUAL(counter, 1, ()); } @@ -77,10 +80,15 @@ UNIT_TEST(TestRouteRebuilding) vector routePoints = kTestRoute; Route masterRoute("dummy", routePoints.begin(), routePoints.end()); size_t counter = 0; - atomic routeBuilded(false); - auto fn = [&routeBuilded](Route const &, IRouter::ResultCode) + bool routeBuilt(false); + mutex waitingMutex; + unique_lock lock(waitingMutex); + condition_variable cv; + auto fn = [&routeBuilt, &waitingMutex, &cv](Route const &, IRouter::ResultCode) { - routeBuilded = true; + lock_guard guard(waitingMutex); + routeBuilt = true; + cv.notify_one(); }; unique_ptr router = make_unique(masterRoute, DummyRouter::NoError, counter); session.SetRouter(move(router), nullptr); @@ -88,11 +96,9 @@ UNIT_TEST(TestRouteRebuilding) // Go along the route. session.BuildRoute(kTestRoute.front(), kTestRoute.back(), fn, nullptr, 0); // Manual check of the routeBuilded mutex to avoid spurious results. - auto time = steady_clock::now() + kTestMaxDuration; - while (steady_clock::now() < time && !routeBuilded) - { - } - TEST(routeBuilded, ("Route was not built.")); + auto time = steady_clock::now() + kRouteBuildingMaxDuration; + cv.wait_until(lock, time, [&routeBuilt, &time]{return routeBuilt || steady_clock::now() > time;}); + TEST(routeBuilt, ("Route was not built.")); location::GpsInfo info; info.m_horizontalAccuracy = 0.01; @@ -110,13 +116,12 @@ UNIT_TEST(TestRouteRebuilding) TEST_EQUAL(counter, 1, ()); // Rebuild route and go in opposite direction. So initiate a route rebuilding flag. + time = steady_clock::now() + kRouteBuildingMaxDuration; counter = 0; - routeBuilded = false; + routeBuilt = false; session.BuildRoute(kTestRoute.front(), kTestRoute.back(), fn, nullptr, 0); - while (steady_clock::now() < time && !routeBuilded) - { - } - TEST(routeBuilded, ("Route was not built.")); + cv.wait_until(lock, time, [&routeBuilt, &time]{return routeBuilt || steady_clock::now() > time;}); + TEST(routeBuilt, ("Route was not built.")); info.m_longitude = 0.; info.m_latitude = 1.; From 4d5b1b99e5f31d9fa404af0fe07311884098f941 Mon Sep 17 00:00:00 2001 From: Lev Dragunov Date: Wed, 11 Nov 2015 19:29:17 +0300 Subject: [PATCH 3/3] PR fixes --- .../routing_tests/routing_session_test.cpp | 74 ++++++++++++------- 1 file changed, 47 insertions(+), 27 deletions(-) diff --git a/routing/routing_tests/routing_session_test.cpp b/routing/routing_tests/routing_session_test.cpp index c5bd3aa258..a2cc532acd 100644 --- a/routing/routing_tests/routing_session_test.cpp +++ b/routing/routing_tests/routing_session_test.cpp @@ -44,6 +44,33 @@ public: static vector kTestRoute = {{0., 1.}, {0., 2.}, {0., 3.}, {0., 4.}}; static auto kRouteBuildingMaxDuration = seconds(30); +class TimedSignal +{ +public: + TimedSignal() : m_flag(false) {} + void Signal() + { + lock_guard guard(m_waitingMutex); + m_flag = true; + m_cv.notify_one(); + } + + bool WaitUntil(steady_clock::time_point const & time) + { + unique_lock lock(m_waitingMutex); + m_cv.wait_until(lock, time, [this, &time] + { + return m_flag || steady_clock::now() > time; + }); + return m_flag; + } + +private: + mutex m_waitingMutex; + condition_variable m_cv; + bool m_flag; +}; + UNIT_TEST(TestRouteBuilding) { RoutingSession session; @@ -51,24 +78,18 @@ UNIT_TEST(TestRouteBuilding) vector routePoints = kTestRoute; Route masterRoute("dummy", routePoints.begin(), routePoints.end()); size_t counter = 0; - bool routeBuilt(false); - mutex waitingMutex; - unique_lock lock(waitingMutex); - condition_variable cv; + TimedSignal timedSignal; unique_ptr router = make_unique(masterRoute, DummyRouter::NoError, counter); session.SetRouter(move(router), nullptr); session.BuildRoute(kTestRoute.front(), kTestRoute.back(), - [&routeBuilt, &waitingMutex, &cv](Route const &, IRouter::ResultCode) + [&timedSignal](Route const &, IRouter::ResultCode) { - lock_guard guard(waitingMutex); - routeBuilt = true; - cv.notify_one(); + timedSignal.Signal(); }, nullptr, 0); // Manual check of the routeBuilded mutex to avoid spurious results. auto const time = steady_clock::now() + kRouteBuildingMaxDuration; - cv.wait_until(lock, time, [&routeBuilt, &time]{return routeBuilt || steady_clock::now() > time;}); - TEST(routeBuilt, ("Route was not built.")); + TEST(timedSignal.WaitUntil(time), ("Route was not built.")); TEST_EQUAL(counter, 1, ()); } @@ -80,25 +101,20 @@ UNIT_TEST(TestRouteRebuilding) vector routePoints = kTestRoute; Route masterRoute("dummy", routePoints.begin(), routePoints.end()); size_t counter = 0; - bool routeBuilt(false); - mutex waitingMutex; - unique_lock lock(waitingMutex); - condition_variable cv; - auto fn = [&routeBuilt, &waitingMutex, &cv](Route const &, IRouter::ResultCode) - { - lock_guard guard(waitingMutex); - routeBuilt = true; - cv.notify_one(); - }; unique_ptr router = make_unique(masterRoute, DummyRouter::NoError, counter); session.SetRouter(move(router), nullptr); // Go along the route. - session.BuildRoute(kTestRoute.front(), kTestRoute.back(), fn, nullptr, 0); + TimedSignal alongTimedSignal; + session.BuildRoute(kTestRoute.front(), kTestRoute.back(), + [&alongTimedSignal](Route const &, IRouter::ResultCode) + { + alongTimedSignal.Signal(); + }, + nullptr, 0); // Manual check of the routeBuilded mutex to avoid spurious results. auto time = steady_clock::now() + kRouteBuildingMaxDuration; - cv.wait_until(lock, time, [&routeBuilt, &time]{return routeBuilt || steady_clock::now() > time;}); - TEST(routeBuilt, ("Route was not built.")); + TEST(alongTimedSignal.WaitUntil(time), ("Route was not built.")); location::GpsInfo info; info.m_horizontalAccuracy = 0.01; @@ -118,10 +134,14 @@ UNIT_TEST(TestRouteRebuilding) // Rebuild route and go in opposite direction. So initiate a route rebuilding flag. time = steady_clock::now() + kRouteBuildingMaxDuration; counter = 0; - routeBuilt = false; - session.BuildRoute(kTestRoute.front(), kTestRoute.back(), fn, nullptr, 0); - cv.wait_until(lock, time, [&routeBuilt, &time]{return routeBuilt || steady_clock::now() > time;}); - TEST(routeBuilt, ("Route was not built.")); + TimedSignal oppositeTimedSignal; + session.BuildRoute(kTestRoute.front(), kTestRoute.back(), + [&oppositeTimedSignal](Route const &, IRouter::ResultCode) + { + oppositeTimedSignal.Signal(); + }, + nullptr, 0); + TEST(oppositeTimedSignal.WaitUntil(time), ("Route was not built.")); info.m_longitude = 0.; info.m_latitude = 1.;