From 7629852c751575bec7f407e2aab4fa4f3c2cde8a Mon Sep 17 00:00:00 2001 From: vng Date: Fri, 25 Sep 2015 14:19:09 +0300 Subject: [PATCH 1/5] [logging] Abort app if LOG error level is greater than LERROR (debug) or LCRITICAL (release). --- android/jni/com/mapswithme/core/logging.cpp | 29 +++++---- base/base.cpp | 18 ++---- base/logging.cpp | 70 +++++---------------- base/logging.hpp | 3 + platform/platform_android.cpp | 2 +- 5 files changed, 43 insertions(+), 79 deletions(-) diff --git a/android/jni/com/mapswithme/core/logging.cpp b/android/jni/com/mapswithme/core/logging.cpp index ab7f2b5ea8..bcda11d230 100644 --- a/android/jni/com/mapswithme/core/logging.cpp +++ b/android/jni/com/mapswithme/core/logging.cpp @@ -1,8 +1,5 @@ #include "logging.hpp" -#include -#include - #include "base/exception.hpp" #include "base/logging.hpp" @@ -11,17 +8,21 @@ #include "platform/file_logging.hpp" #include "platform/platform.hpp" +#include +#include +#include + namespace jni { using namespace my; -void AndroidLogMessage(LogLevel l, SrcPoint const & src, string const & s) +void AndroidMessage(LogLevel level, SrcPoint const & src, string const & s) { android_LogPriority pr = ANDROID_LOG_SILENT; - switch (l) + switch (level) { case LINFO: pr = ANDROID_LOG_INFO; break; case LDEBUG: pr = ANDROID_LOG_DEBUG; break; @@ -34,24 +35,30 @@ void AndroidLogMessage(LogLevel l, SrcPoint const & src, string const & s) __android_log_write(pr, "MapsWithMe_JNI", out.c_str()); } +void AndroidLogMessage(LogLevel level, SrcPoint const & src, string const & s) +{ + AndroidMessage(level, src, s); + CHECK(level < g_LogAbortLevel, ("Abort. Log level is too serious", level)); +} + void AndroidAssertMessage(SrcPoint const & src, string const & s) { -#if defined(MWM_LOG_TO_FILE) - LogMessageFile(LERROR, src, s); +#ifdef MWM_LOG_TO_FILE + LogMessageFile(LCRITICAL, src, s); #else - AndroidLogMessage(LERROR, src, s); + AndroidMessage(LCRITICAL, src, s); #endif #ifdef DEBUG - assert(false); + assert(false); #else - MYTHROW(RootException, (s)); + std::abort(); #endif } void InitSystemLog() { -#if defined(MWM_LOG_TO_FILE) +#ifdef MWM_LOG_TO_FILE SetLogMessageFn(&LogMessageFile); #else SetLogMessageFn(&AndroidLogMessage); diff --git a/base/base.cpp b/base/base.cpp index 3dcd92d61d..dc93a19ea2 100644 --- a/base/base.cpp +++ b/base/base.cpp @@ -6,29 +6,21 @@ #include "std/iostream.hpp" #include +#include -#ifdef OMIM_OS_TIZEN - #include -#endif namespace my { void OnAssertFailedDefault(SrcPoint const & srcPoint, string const & msg) { -#ifdef OMIM_OS_TIZEN - AppLog("ASSERT FAILED%s:%d:%s", srcPoint.FileName(), srcPoint.Line(), msg.c_str()); - AppAssert(false); - -#else - std::cerr << "ASSERT FAILED\n" << srcPoint.FileName() << ":" << srcPoint.Line() << "\n" - << msg << endl; + std::cerr << "ASSERT FAILED" << endl + << srcPoint.FileName() << ":" << srcPoint.Line() << endl + << msg << endl; #ifdef DEBUG assert(false); #else - MYTHROW(RootException, (msg)); -#endif - + std::abort(); #endif } diff --git a/base/logging.cpp b/base/logging.cpp index fbf30b9539..0ac3b05cac 100644 --- a/base/logging.cpp +++ b/base/logging.cpp @@ -1,12 +1,12 @@ #include "base/assert.hpp" #include "base/logging.hpp" #include "base/macros.hpp" -#include "base/timer.hpp" -#include "base/thread.hpp" #include "base/mutex.hpp" +#include "base/thread.hpp" +#include "base/timer.hpp" -#include "std/iostream.hpp" #include "std/iomanip.hpp" +#include "std/iostream.hpp" #include "std/mutex.hpp" #include "std/sstream.hpp" #include "std/target_os.hpp" @@ -15,41 +15,6 @@ namespace my { - void LogCheckIfErrorLevel(LogLevel level) - { -#ifdef DEBUG - if (level >= LERROR) -#else - if (level >= LCRITICAL) -#endif - { - CHECK(false, ("Error level is too serious", level)); - } - } - -#ifdef OMIM_OS_TIZEN -#include - void LogMessageDefault(LogLevel level, SrcPoint const & srcPoint, string const & msg) - { - ostringstream out; - out << DebugPrint(srcPoint) << msg << endl; - switch (level) - { - case LDEBUG: - AppLogDebug(out.str().c_str()); - break; - case LINFO: - case LWARNING: - AppLog(out.str().c_str()); - break; - case LERROR: - case LCRITICAL: - AppLogException(out.str().c_str()); - } - - } -#else - class LogHelper { int m_threadsCount; @@ -69,8 +34,6 @@ namespace my size_t m_lens[5]; public: - threads::Mutex m_mutex; - LogHelper() : m_threadsCount(0) { m_names[0] = "DEBUG"; m_lens[0] = 5; @@ -92,39 +55,36 @@ namespace my } }; + mutex g_logMutex; + void LogMessageDefault(LogLevel level, SrcPoint const & srcPoint, string const & msg) { - static LogHelper logger; + lock_guard lock(g_logMutex); + UNUSED_VALUE(lock); - threads::MutexGuard guard(logger.m_mutex); - UNUSED_VALUE(guard); + static LogHelper logger; ostringstream out; logger.WriteProlog(out, level); out << DebugPrint(srcPoint) << msg << endl; - std::cerr << out.str(); - + CHECK(level < g_LogAbortLevel, ("Abort. Log level is too serious", level)); } - void LogMessageTests(LogLevel level, SrcPoint const & srcPoint, string const & msg) + + void LogMessageTests(LogLevel level, SrcPoint const &, string const & msg) { - static mutex mtx; - lock_guard lock(mtx); + lock_guard lock(g_logMutex); + UNUSED_VALUE(lock); ostringstream out; out << msg << endl; std::cerr << out.str(); -#ifdef OMIM_OS_WINDOWS - OutputDebugStringA(out.str().c_str()); -#endif - LogCheckIfErrorLevel(level); + CHECK(level < g_LogAbortLevel, ("Abort. Log level is too serious", level)); } -#endif - LogMessageFn LogMessage = &LogMessageDefault; LogMessageFn SetLogMessageFn(LogMessageFn fn) @@ -135,7 +95,9 @@ namespace my #ifdef DEBUG LogLevel g_LogLevel = LDEBUG; + LogLevel g_LogAbortLevel = LERROR; #else LogLevel g_LogLevel = LINFO; + LogLevel g_LogAbortLevel = LCRITICAL; #endif } diff --git a/base/logging.hpp b/base/logging.hpp index f5b248da8f..384d33d7d3 100644 --- a/base/logging.hpp +++ b/base/logging.hpp @@ -18,9 +18,12 @@ namespace my extern LogMessageFn LogMessage; extern LogLevel g_LogLevel; + extern LogLevel g_LogAbortLevel; /// @return Pointer to previous message function. LogMessageFn SetLogMessageFn(LogMessageFn fn); + + void LogMessageDefault(LogLevel level, SrcPoint const & srcPoint, string const & msg); void LogMessageTests(LogLevel level, SrcPoint const & srcPoint, string const & msg); } diff --git a/platform/platform_android.cpp b/platform/platform_android.cpp index ca366a4095..df2715c15b 100644 --- a/platform/platform_android.cpp +++ b/platform/platform_android.cpp @@ -182,7 +182,7 @@ ModelReader * Platform::GetReader(string const & file, string const & searchScop } } - LOG(LERROR, ("Can't get reader for:", file)); + LOG(LWARNING, ("Can't get reader for:", file)); MYTHROW(FileAbsentException, ("File not found", file)); return 0; } From c56f4c20b946bf62831e917a695d4e6c7861fdd9 Mon Sep 17 00:00:00 2001 From: vng Date: Fri, 25 Sep 2015 16:09:20 +0300 Subject: [PATCH 2/5] [mwm set] Do not create MwmHandle instantly after registering map. --- generator/routing_generator.cpp | 2 +- indexer/index.cpp | 2 +- indexer/index.hpp | 2 +- indexer/indexer_tests/index_test.cpp | 6 ++-- indexer/indexer_tests/mwm_set_test.cpp | 2 +- indexer/mwm_set.cpp | 12 ++++---- indexer/mwm_set.hpp | 4 +-- map/feature_vec_model.cpp | 17 ++++++----- map/feature_vec_model.hpp | 2 +- map/framework.cpp | 16 +++++----- map/framework.hpp | 2 +- map/mwm_tests/mwm_index_test.cpp | 7 +++-- .../routing_tests/routing_mapping_test.cpp | 12 ++++---- search/integration_tests/retrieval_test.cpp | 30 +++++++++---------- search/search_tests/locality_finder_test.cpp | 6 ++-- 15 files changed, 63 insertions(+), 59 deletions(-) diff --git a/generator/routing_generator.cpp b/generator/routing_generator.cpp index a699e4110f..aa3d39b178 100644 --- a/generator/routing_generator.cpp +++ b/generator/routing_generator.cpp @@ -272,7 +272,7 @@ void BuildRoutingIndex(string const & baseDir, string const & countryName, strin } FeatureType ft; - Index::FeaturesLoaderGuard loader(index, p.first.GetId()); + Index::FeaturesLoaderGuard loader(index, p.first); loader.GetFeatureByIndex(fID, ft); ft.ParseGeometry(FeatureType::BEST_GEOMETRY); diff --git a/indexer/index.cpp b/indexer/index.cpp index 508e0306c0..1592715dff 100644 --- a/indexer/index.cpp +++ b/indexer/index.cpp @@ -63,7 +63,7 @@ unique_ptr Index::CreateValue(MwmInfo & info) const return unique_ptr(move(p)); } -pair Index::RegisterMap(LocalCountryFile const & localFile) +pair Index::RegisterMap(LocalCountryFile const & localFile) { auto result = Register(localFile); if (result.first.IsAlive() && result.second == MwmSet::RegResult::Success) diff --git a/indexer/index.hpp b/indexer/index.hpp index 44057132dd..eab3b90c79 100644 --- a/indexer/index.hpp +++ b/indexer/index.hpp @@ -75,7 +75,7 @@ public: /// Registers a new map. - WARN_UNUSED_RESULT pair RegisterMap( + WARN_UNUSED_RESULT pair RegisterMap( platform::LocalCountryFile const & localFile); /// Deregisters a map from internal records. diff --git a/indexer/indexer_tests/index_test.cpp b/indexer/indexer_tests/index_test.cpp index 7c5f5a2332..1b30199d23 100644 --- a/indexer/indexer_tests/index_test.cpp +++ b/indexer/indexer_tests/index_test.cpp @@ -113,7 +113,7 @@ UNIT_TEST(Index_MwmStatusNotifications) TEST(p.first.IsAlive(), ()); TEST_EQUAL(MwmSet::RegResult::Success, p.second, ()); observer.CheckExpectations(); - localFileV1Id = p.first.GetId(); + localFileV1Id = p.first; } // Checks that map can't registered twice. @@ -122,7 +122,7 @@ UNIT_TEST(Index_MwmStatusNotifications) TEST(p.first.IsAlive(), ()); TEST_EQUAL(MwmSet::RegResult::VersionAlreadyExists, p.second, ()); observer.CheckExpectations(); - TEST_EQUAL(localFileV1Id, p.first.GetId(), ()); + TEST_EQUAL(localFileV1Id, p.first, ()); } // Checks that observers are notified when map is updated. @@ -134,7 +134,7 @@ UNIT_TEST(Index_MwmStatusNotifications) TEST(p.first.IsAlive(), ()); TEST_EQUAL(MwmSet::RegResult::Success, p.second, ()); observer.CheckExpectations(); - localFileV2Id = p.first.GetId(); + localFileV2Id = p.first; TEST_NOT_EQUAL(localFileV1Id, localFileV2Id, ()); } diff --git a/indexer/indexer_tests/mwm_set_test.cpp b/indexer/indexer_tests/mwm_set_test.cpp index 76c25c9867..372bab619e 100644 --- a/indexer/indexer_tests/mwm_set_test.cpp +++ b/indexer/indexer_tests/mwm_set_test.cpp @@ -133,7 +133,7 @@ UNIT_TEST(MwmSetLockAndIdTest) { auto p = mwmSet.Register(LocalCountryFile::MakeForTesting("4")); - MwmSet::MwmHandle const & handle = p.first; + MwmSet::MwmHandle handle = mwmSet.GetMwmHandleById(p.first); TEST(handle.IsAlive(), ()); TEST_EQUAL(MwmSet::RegResult::Success, p.second, ("Can't register test mwm 4")); TEST_EQUAL(MwmInfo::STATUS_REGISTERED, handle.GetInfo()->GetStatus(), ()); diff --git a/indexer/mwm_set.cpp b/indexer/mwm_set.cpp index e5ee51cd01..71d8bcef0e 100644 --- a/indexer/mwm_set.cpp +++ b/indexer/mwm_set.cpp @@ -83,7 +83,7 @@ MwmSet::MwmId MwmSet::GetMwmIdByCountryFileImpl(CountryFile const & countryFile) return MwmId(it->second.back()); } -pair MwmSet::Register(LocalCountryFile const & localFile) +pair MwmSet::Register(LocalCountryFile const & localFile) { lock_guard lock(m_lock); @@ -108,26 +108,26 @@ pair MwmSet::Register(LocalCountryFile con LOG(LINFO, ("Updating already registered mwm:", name)); info->SetStatus(MwmInfo::STATUS_REGISTERED); info->m_file = localFile; - return make_pair(GetLock(id), RegResult::VersionAlreadyExists); + return make_pair(id, RegResult::VersionAlreadyExists); } LOG(LWARNING, ("Trying to add too old (", localFile.GetVersion(), ") mwm (", name, "), current version:", info->GetVersion())); - return make_pair(MwmHandle(), RegResult::VersionTooOld); + return make_pair(MwmId(), RegResult::VersionTooOld); } -pair MwmSet::RegisterImpl(LocalCountryFile const & localFile) +pair MwmSet::RegisterImpl(LocalCountryFile const & localFile) { // This function can throw an exception for a bad mwm file. shared_ptr info(CreateInfo(localFile)); if (!info) - return make_pair(MwmHandle(), RegResult::UnsupportedFileFormat); + return make_pair(MwmId(), RegResult::UnsupportedFileFormat); info->m_file = localFile; info->SetStatus(MwmInfo::STATUS_REGISTERED); m_info[localFile.GetCountryName()].push_back(info); - return make_pair(GetLock(MwmId(info)), RegResult::Success); + return make_pair(MwmId(info), RegResult::Success); } bool MwmSet::DeregisterImpl(MwmId const & id) diff --git a/indexer/mwm_set.hpp b/indexer/mwm_set.hpp index fb705ce7de..fb847e5f10 100644 --- a/indexer/mwm_set.hpp +++ b/indexer/mwm_set.hpp @@ -164,11 +164,11 @@ public: /// are older than the localFile (in this case mwm handle will point /// to just-registered file). protected: - WARN_UNUSED_RESULT pair RegisterImpl( + WARN_UNUSED_RESULT pair RegisterImpl( platform::LocalCountryFile const & localFile); public: - WARN_UNUSED_RESULT pair Register( + WARN_UNUSED_RESULT pair Register( platform::LocalCountryFile const & localFile); //@} diff --git a/map/feature_vec_model.cpp b/map/feature_vec_model.cpp index f28c8e65e1..65812a5c54 100644 --- a/map/feature_vec_model.cpp +++ b/map/feature_vec_model.cpp @@ -44,7 +44,7 @@ void FeaturesFetcher::InitClassificator() } } -pair FeaturesFetcher::RegisterMap( +pair FeaturesFetcher::RegisterMap( LocalCountryFile const & localFile) { try @@ -54,17 +54,20 @@ pair FeaturesFetcher::RegisterMap( { LOG(LWARNING, ("Can't add map", localFile.GetCountryName(), "Probably it's already added or has newer data version.")); - return result; } - MwmSet::MwmHandle & handle = result.first; - ASSERT(handle.IsAlive(), ("Mwm lock invariant violation.")); - m_rect.Add(handle.GetInfo()->m_limitRect); + else + { + MwmSet::MwmId const & id = result.first; + ASSERT(id.IsAlive(), ()); + m_rect.Add(id.GetInfo()->m_limitRect); + } + return result; } catch (RootException const & ex) { - LOG(LERROR, ("IO error while adding ", localFile.GetCountryName(), " map. ", ex.Msg())); - return make_pair(MwmSet::MwmHandle(), MwmSet::RegResult::BadFile); + LOG(LERROR, ("IO error while adding", localFile.GetCountryName(), "map.", ex.Msg())); + return make_pair(MwmSet::MwmId(), MwmSet::RegResult::BadFile); } } diff --git a/map/feature_vec_model.hpp b/map/feature_vec_model.hpp index f7e21a575a..fce5e8b0e4 100644 --- a/map/feature_vec_model.hpp +++ b/map/feature_vec_model.hpp @@ -46,7 +46,7 @@ class FeaturesFetcher : public Index::Observer } /// Registers a new map. - WARN_UNUSED_RESULT pair RegisterMap( + WARN_UNUSED_RESULT pair RegisterMap( platform::LocalCountryFile const & localFile); /// Deregisters a map denoted by file from internal records. diff --git a/map/framework.cpp b/map/framework.cpp index 844be1328d..2667c8172a 100644 --- a/map/framework.cpp +++ b/map/framework.cpp @@ -102,7 +102,7 @@ namespace char const kRouterTypeKey[] = "router"; } -pair Framework::RegisterMap( +pair Framework::RegisterMap( LocalCountryFile const & localFile) { LOG(LINFO, ("Loading map:", localFile.GetCountryName())); @@ -447,10 +447,10 @@ void Framework::UpdateLatestCountryFile(LocalCountryFile const & localFile) return; // Add downloaded map. - auto result = m_model.RegisterMap(localFile); - MwmSet::MwmHandle const & handle = result.first; - if (handle.IsAlive()) - InvalidateRect(handle.GetInfo()->m_limitRect, true /* doForceUpdate */); + auto p = m_model.RegisterMap(localFile); + MwmSet::MwmId const & id = p.first; + if (id.IsAlive()) + InvalidateRect(id.GetInfo()->m_limitRect, true /* doForceUpdate */); GetSearchEngine()->ClearViewportsCache(); } @@ -479,9 +479,9 @@ void Framework::RegisterAllMaps() if (p.second != MwmSet::RegResult::Success) continue; - MwmSet::MwmHandle const & handle = p.first; - ASSERT(handle.IsAlive(), ()); - minFormat = min(minFormat, static_cast(handle.GetInfo()->m_version.format)); + MwmSet::MwmId const & id = p.first; + ASSERT(id.IsAlive(), ()); + minFormat = min(minFormat, static_cast(id.GetInfo()->m_version.format)); } m_countryTree.Init(maps); diff --git a/map/framework.hpp b/map/framework.hpp index 0fd1caecf4..06f972ffaf 100644 --- a/map/framework.hpp +++ b/map/framework.hpp @@ -203,7 +203,7 @@ public: void DeregisterAllMaps(); /// Registers a local map file in internal indexes. - pair RegisterMap( + pair RegisterMap( platform::LocalCountryFile const & localFile); //@} diff --git a/map/mwm_tests/mwm_index_test.cpp b/map/mwm_tests/mwm_index_test.cpp index f2ec06de68..3b299b4d95 100644 --- a/map/mwm_tests/mwm_index_test.cpp +++ b/map/mwm_tests/mwm_index_test.cpp @@ -42,10 +42,11 @@ bool RunTest(string const & countryFileName, int lowS, int highS) auto p = src.RegisterMap(platform::LocalCountryFile::MakeForTesting(countryFileName)); if (p.second != MwmSet::RegResult::Success) return false; - MwmSet::MwmHandle const & handle = p.first; - ASSERT(handle.IsAlive(), ()); - version::Format const version = handle.GetInfo()->m_version.format; + MwmSet::MwmId const & id = p.first; + ASSERT(id.IsAlive(), ()); + + version::Format const version = id.GetInfo()->m_version.format; if (version == version::unknownFormat) return false; diff --git a/routing/routing_tests/routing_mapping_test.cpp b/routing/routing_tests/routing_mapping_test.cpp index 57cdcb7674..7d4c977250 100644 --- a/routing/routing_tests/routing_mapping_test.cpp +++ b/routing/routing_tests/routing_mapping_test.cpp @@ -63,7 +63,7 @@ private: ScopedFile m_testRoutingFile; LocalCountryFile m_localFile; TestMwmSet m_testSet; - pair m_result; + pair m_result; }; UNIT_TEST(RoutingMappingCountryFileLockTest) @@ -72,10 +72,10 @@ UNIT_TEST(RoutingMappingCountryFileLockTest) { RoutingMapping testMapping(generator.GetCountryName(), (&generator.GetMwmSet())); TEST(testMapping.IsValid(), ()); - TEST_EQUAL(generator.GetNumRefs(), 2, ()); + TEST_EQUAL(generator.GetNumRefs(), 1, ()); } // Routing mapping must unlock the file after destruction. - TEST_EQUAL(generator.GetNumRefs(), 1, ()); + TEST_EQUAL(generator.GetNumRefs(), 0, ()); } UNIT_TEST(IndexManagerLockManagementTest) @@ -87,13 +87,13 @@ UNIT_TEST(IndexManagerLockManagementTest) { auto testMapping = manager.GetMappingByName(fileName); TEST(testMapping->IsValid(), ()); - TEST_EQUAL(generator.GetNumRefs(), 2, ()); + TEST_EQUAL(generator.GetNumRefs(), 1, ()); } // We freed mapping, but it still persists inside the manager cache. - TEST_EQUAL(generator.GetNumRefs(), 2, ()); + TEST_EQUAL(generator.GetNumRefs(), 1, ()); // Test cache clearing. manager.Clear(); - TEST_EQUAL(generator.GetNumRefs(), 1, ()); + TEST_EQUAL(generator.GetNumRefs(), 0, ()); } } // namespace diff --git a/search/integration_tests/retrieval_test.cpp b/search/integration_tests/retrieval_test.cpp index 2a6f3d3dab..25ef9f9002 100644 --- a/search/integration_tests/retrieval_test.cpp +++ b/search/integration_tests/retrieval_test.cpp @@ -108,8 +108,8 @@ UNIT_TEST(Retrieval_Smoke) Index index; auto p = index.RegisterMap(file); - auto & handle = p.first; - TEST(handle.IsAlive(), ()); + auto & id = p.first; + TEST(id.IsAlive(), ()); TEST_EQUAL(p.second, MwmSet::RegResult::Success, ()); search::SearchQueryParams params; @@ -122,7 +122,7 @@ UNIT_TEST(Retrieval_Smoke) // Retrieve all (100) whiskey bars from the mwm. { - TestCallback callback(handle.GetId()); + TestCallback callback(id); retrieval.Init(index, infos, m2::RectD(m2::PointD(0, 0), m2::PointD(1, 1)), params, search::Retrieval::Limits()); @@ -130,14 +130,14 @@ UNIT_TEST(Retrieval_Smoke) TEST(callback.WasTriggered(), ()); TEST_EQUAL(100, callback.Offsets().size(), ()); - TestCallback dummyCallback(handle.GetId()); + TestCallback dummyCallback(id); retrieval.Go(dummyCallback); TEST(!dummyCallback.WasTriggered(), ()); } // Retrieve all whiskey bars from the left-bottom 5 x 5 square. { - TestCallback callback(handle.GetId()); + TestCallback callback(id); search::Retrieval::Limits limits; limits.SetMaxViewportScale(9.0); @@ -150,7 +150,7 @@ UNIT_TEST(Retrieval_Smoke) // Retrieve exactly 8 whiskey bars from the center. { - TestCallback callback(handle.GetId()); + TestCallback callback(id); search::Retrieval::Limits limits; limits.SetMaxNumFeatures(8); @@ -192,17 +192,17 @@ UNIT_TEST(Retrieval_3Mwms) Index index; auto mskP = index.RegisterMap(msk); - auto & mskHandle = mskP.first; + auto & mskId = mskP.first; auto mtvP = index.RegisterMap(mtv); - auto & mtvHandle = mtvP.first; + auto & mtvId = mtvP.first; auto zrhP = index.RegisterMap(zrh); - auto & zrhHandle = zrhP.first; + auto & zrhId = zrhP.first; - TEST(mskHandle.IsAlive(), ()); - TEST(mtvHandle.IsAlive(), ()); - TEST(zrhHandle.IsAlive(), ()); + TEST(mskId.IsAlive(), ()); + TEST(mtvId.IsAlive(), ()); + TEST(zrhId.IsAlive(), ()); search::SearchQueryParams params; InitParams("mtv", params); @@ -213,7 +213,7 @@ UNIT_TEST(Retrieval_3Mwms) search::Retrieval retrieval; { - TestCallback callback(mskHandle.GetId()); + TestCallback callback(mskId); search::Retrieval::Limits limits; limits.SetMaxNumFeatures(1); @@ -225,7 +225,7 @@ UNIT_TEST(Retrieval_3Mwms) } { - MultiMwmCallback callback({mskHandle.GetId(), mtvHandle.GetId(), zrhHandle.GetId()}); + MultiMwmCallback callback({mskId, mtvId, zrhId}); search::Retrieval::Limits limits; limits.SetMaxNumFeatures(10 /* more than total number of features in all these mwms */); @@ -237,7 +237,7 @@ UNIT_TEST(Retrieval_3Mwms) } { - MultiMwmCallback callback({mskHandle.GetId(), mtvHandle.GetId(), zrhHandle.GetId()}); + MultiMwmCallback callback({mskId, mtvId, zrhId}); search::Retrieval::Limits limits; retrieval.Init(index, infos, m2::RectD(m2::PointD(-1.0, -1.0), m2::PointD(1.0, 1.0)), params, diff --git a/search/search_tests/locality_finder_test.cpp b/search/search_tests/locality_finder_test.cpp index 48314862ae..bcb50e6227 100644 --- a/search/search_tests/locality_finder_test.cpp +++ b/search/search_tests/locality_finder_test.cpp @@ -59,10 +59,10 @@ UNIT_TEST(LocalityFinder) auto const p = index.Register(world); TEST_EQUAL(MwmSet::RegResult::Success, p.second, ()); - MwmSet::MwmHandle const & handle = p.first; - TEST(handle.IsAlive(), ()); + MwmSet::MwmId const & id = p.first; + TEST(id.IsAlive(), ()); - rect = handle.GetId().GetInfo()->m_limitRect; + rect = id.GetInfo()->m_limitRect; } catch (RootException const & ex) { From 25aaa05cefa9ecdd4ce6e7893786b4bf0eec5366 Mon Sep 17 00:00:00 2001 From: vng Date: Fri, 25 Sep 2015 16:22:48 +0300 Subject: [PATCH 3/5] [mwm set] Handle file system exceptions when building feature offsets index. --- indexer/features_offsets_table.cpp | 2 +- .../features_offsets_table_test.cpp | 2 +- indexer/mwm_set.cpp | 14 +++++- map/map_tests/map_tests.pro | 1 + map/map_tests/mwm_set_test.cpp | 49 +++++++++++++++++++ platform/local_country_file_utils.cpp | 11 +++-- platform/local_country_file_utils.hpp | 8 +-- platform/platform.hpp | 2 +- .../local_country_file_tests.cpp | 6 +-- routing/osrm2feature_map.cpp | 1 + storage/storage_tests/storage_tests.cpp | 2 +- 11 files changed, 82 insertions(+), 16 deletions(-) create mode 100644 map/map_tests/mwm_set_test.cpp diff --git a/indexer/features_offsets_table.cpp b/indexer/features_offsets_table.cpp index 2850a73cde..2b6d9e19fe 100644 --- a/indexer/features_offsets_table.cpp +++ b/indexer/features_offsets_table.cpp @@ -71,7 +71,7 @@ namespace feature { LOG(LINFO, ("Creating features offset table file", storePath)); - VERIFY(CountryIndexes::PreparePlaceOnDisk(localFile), ()); + CountryIndexes::PreparePlaceOnDisk(localFile); Builder builder; FeaturesVector::ForEachOffset(cont.GetReader(DATA_FILE_TAG), [&builder] (uint32_t offset) diff --git a/indexer/indexer_tests/features_offsets_table_test.cpp b/indexer/indexer_tests/features_offsets_table_test.cpp index d08cca7486..f995cd673c 100644 --- a/indexer/indexer_tests/features_offsets_table_test.cpp +++ b/indexer/indexer_tests/features_offsets_table_test.cpp @@ -97,7 +97,7 @@ namespace feature FilesContainerR baseContainer(pl.GetReader("minsk-pass" DATA_FILE_EXTENSION)); LocalCountryFile localFile = LocalCountryFile::MakeForTesting(testFileName); - TEST(CountryIndexes::PreparePlaceOnDisk(localFile), ()); + CountryIndexes::PreparePlaceOnDisk(localFile); string const indexFile = CountryIndexes::GetPath(localFile, CountryIndexes::Index::Offsets); FileWriter::DeleteFileX(indexFile); diff --git a/indexer/mwm_set.cpp b/indexer/mwm_set.cpp index 71d8bcef0e..1079ba19ff 100644 --- a/indexer/mwm_set.cpp +++ b/indexer/mwm_set.cpp @@ -4,6 +4,7 @@ #include "defines.hpp" #include "base/assert.hpp" +#include "base/exception.hpp" #include "base/logging.hpp" #include "base/stl_add.hpp" @@ -214,7 +215,18 @@ unique_ptr MwmSet::LockValueImpl(MwmId const & id) } } - return CreateValue(*info); + try + { + return CreateValue(*info); + } + catch (exception const & ex) + { + LOG(LERROR, ("Can't create MWMValue for", info->GetCountryName(), "Reason", ex.what())); + + --info->m_numRefs; + DeregisterImpl(id); + return nullptr; + } } void MwmSet::UnlockValue(MwmId const & id, unique_ptr && p) diff --git a/map/map_tests/map_tests.pro b/map/map_tests/map_tests.pro index 17df03d4dc..50d789ea1a 100644 --- a/map/map_tests/map_tests.pro +++ b/map/map_tests/map_tests.pro @@ -36,6 +36,7 @@ SOURCES += \ kmz_unarchive_test.cpp \ mwm_url_tests.cpp \ navigator_test.cpp \ + mwm_set_test.cpp \ !linux* { SOURCES += working_time_tests.cpp \ diff --git a/map/map_tests/mwm_set_test.cpp b/map/map_tests/mwm_set_test.cpp new file mode 100644 index 0000000000..3ea80b0536 --- /dev/null +++ b/map/map_tests/mwm_set_test.cpp @@ -0,0 +1,49 @@ +#include "testing/testing.hpp" + +#include "indexer/index.hpp" + +#include "platform/local_country_file_utils.hpp" +#include "platform/platform.hpp" + +#ifndef OMIM_OS_WINDOWS +#include +#endif + + +using namespace platform; +using namespace my; + +#ifndef OMIM_OS_WINDOWS +UNIT_TEST(MwmSet_FileSystemErrors) +{ + string const dir = GetPlatform().WritableDir(); + + CountryFile file("minsk-pass"); + LocalCountryFile localFile(dir, file, 0); + TEST(CountryIndexes::DeleteFromDisk(localFile), ()); + + LogLevel oldLevel = g_LogAbortLevel; + g_LogAbortLevel = LCRITICAL; + + // Remove writable permission. + int const readOnlyMode = S_IRUSR | S_IRGRP | S_IROTH | S_IXUSR | S_IXGRP | S_IXOTH; + TEST_EQUAL(chmod(dir.c_str(), readOnlyMode), 0, ()); + + Index index; + auto p = index.RegisterMap(localFile); + TEST_EQUAL(p.second, Index::RegResult::Success, ()); + + TEST(index.GetMwmIdByCountryFile(file) != Index::MwmId(), ()); + + TEST(!index.GetMwmHandleById(p.first).IsAlive(), ()); + + vector> infos; + index.GetMwmsInfo(infos); + TEST(infos.empty(), ()); + + // Restore writable permission. + TEST_EQUAL(chmod(dir.c_str(), readOnlyMode | S_IWUSR), 0, ()); + + g_LogAbortLevel = oldLevel; +} +#endif diff --git a/platform/local_country_file_utils.cpp b/platform/local_country_file_utils.cpp index 6519274861..19bcc28718 100644 --- a/platform/local_country_file_utils.cpp +++ b/platform/local_country_file_utils.cpp @@ -239,9 +239,11 @@ ModelReader * GetCountryReader(platform::LocalCountryFile const & file, MapOptio } // static -bool CountryIndexes::PreparePlaceOnDisk(LocalCountryFile const & localFile) +void CountryIndexes::PreparePlaceOnDisk(LocalCountryFile const & localFile) { - return MkDirChecked(IndexesDir(localFile)); + string const dir = IndexesDir(localFile); + if (!MkDirChecked(dir)) + MYTHROW(FileSystemException, ("Can't create directory", dir)); } // static @@ -302,6 +304,8 @@ string CountryIndexes::IndexesDir(LocalCountryFile const & localFile) string dir = localFile.GetDirectory(); CountryFile const & file = localFile.GetCountryFile(); + /// @todo It's a temporary code until we will put fIndex->fOffset into mwm container. + /// IndexesDir should not throw any exceptions. if (dir.empty()) { // Local file is stored in resources. Need to prepare index folder in the writable directory. @@ -309,7 +313,8 @@ string CountryIndexes::IndexesDir(LocalCountryFile const & localFile) ASSERT_GREATER(version, 0, ()); dir = my::JoinFoldersToPath(GetPlatform().WritableDir(), strings::to_string(version)); - VERIFY(MkDirChecked(dir), ()); + if (!MkDirChecked(dir)) + MYTHROW(FileSystemException, ("Can't create directory", dir)); } return my::JoinFoldersToPath(dir, file.GetNameWithoutExt()); diff --git a/platform/local_country_file_utils.hpp b/platform/local_country_file_utils.hpp index 660dcd65c3..739a9d35ba 100644 --- a/platform/local_country_file_utils.hpp +++ b/platform/local_country_file_utils.hpp @@ -62,10 +62,10 @@ public: Offsets }; - // Prepares (if necessary) directory for country indexes. Local file - // should point to existing local country files. Returns true on - // success, false otherwise. - static bool PreparePlaceOnDisk(LocalCountryFile const & localFile); + /// Prepares (if necessary) directory for country indexes. Local file + /// should point to existing local country files. + /// @throw FileSystemException if any file system error occured. + static void PreparePlaceOnDisk(LocalCountryFile const & localFile); // Removes country indexes from disk including containing directory. static bool DeleteFromDisk(LocalCountryFile const & localFile); diff --git a/platform/platform.hpp b/platform/platform.hpp index 16d369d27b..be5e8e4db1 100644 --- a/platform/platform.hpp +++ b/platform/platform.hpp @@ -15,7 +15,7 @@ #include "defines.hpp" DECLARE_EXCEPTION(FileAbsentException, RootException); -DECLARE_EXCEPTION(NotImplementedException, RootException); +DECLARE_EXCEPTION(FileSystemException, RootException); namespace platform { diff --git a/platform/platform_tests/local_country_file_tests.cpp b/platform/platform_tests/local_country_file_tests.cpp index c98dd28009..478bfd35dd 100644 --- a/platform/platform_tests/local_country_file_tests.cpp +++ b/platform/platform_tests/local_country_file_tests.cpp @@ -313,8 +313,7 @@ UNIT_TEST(LocalCountryFile_CountryIndexes) TEST_EQUAL( my::JoinFoldersToPath(germanyLocalFile.GetDirectory(), germanyFile.GetNameWithoutExt()), CountryIndexes::IndexesDir(germanyLocalFile), ()); - TEST(CountryIndexes::PreparePlaceOnDisk(germanyLocalFile), - ("Can't prepare place for:", germanyLocalFile)); + CountryIndexes::PreparePlaceOnDisk(germanyLocalFile); string const bitsPath = CountryIndexes::GetPath(germanyLocalFile, CountryIndexes::Index::Bits); TEST(!Platform::IsFileExistsByFullPath(bitsPath), (bitsPath)); @@ -344,9 +343,8 @@ UNIT_TEST(LocalCountryFile_DoNotDeleteUserFiles) CountryFile germanyFile("Germany"); LocalCountryFile germanyLocalFile(testDir.GetFullPath(), germanyFile, 101010 /* version */); + CountryIndexes::PreparePlaceOnDisk(germanyLocalFile); - TEST(CountryIndexes::PreparePlaceOnDisk(germanyLocalFile), - ("Can't prepare place for:", germanyLocalFile)); string const userFilePath = my::JoinFoldersToPath(CountryIndexes::IndexesDir(germanyLocalFile), "user-data.txt"); { diff --git a/routing/osrm2feature_map.cpp b/routing/osrm2feature_map.cpp index 9ecc62fd10..beb889908c 100644 --- a/routing/osrm2feature_map.cpp +++ b/routing/osrm2feature_map.cpp @@ -388,6 +388,7 @@ void OsrmFtSegBackwardIndex::Construct(OsrmFtSegMapping & mapping, uint32_t maxN if (m_oldFormat) LOG(LINFO, ("Using old format index for", localFile.GetCountryName())); + CountryIndexes::PreparePlaceOnDisk(localFile); string const bitsFileName = CountryIndexes::GetPath(localFile, CountryIndexes::Index::Bits); string const nodesFileName = CountryIndexes::GetPath(localFile, CountryIndexes::Index::Nodes); diff --git a/storage/storage_tests/storage_tests.cpp b/storage/storage_tests/storage_tests.cpp index 42cd6b6ee4..733aa7c452 100644 --- a/storage/storage_tests/storage_tests.cpp +++ b/storage/storage_tests/storage_tests.cpp @@ -598,7 +598,7 @@ UNIT_TEST(StorageTest_DeleteCountry) LocalCountryFile file = LocalCountryFile::MakeForTesting("Wonderland"); TEST_EQUAL(MapOptions::MapWithCarRouting, file.GetFiles(), ()); - TEST(CountryIndexes::PreparePlaceOnDisk(file), ()); + CountryIndexes::PreparePlaceOnDisk(file); string const bitsPath = CountryIndexes::GetPath(file, CountryIndexes::Index::Bits); { FileWriter writer(bitsPath); From 5fc4681e5449c00ba25cfc0882cac95155d76a33 Mon Sep 17 00:00:00 2001 From: vng Date: Fri, 25 Sep 2015 16:33:57 +0300 Subject: [PATCH 4/5] Fixed uint32_t serialization bug. --- routing/cross_routing_context.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/routing/cross_routing_context.cpp b/routing/cross_routing_context.cpp index 090c9254f9..317b854631 100644 --- a/routing/cross_routing_context.cpp +++ b/routing/cross_routing_context.cpp @@ -57,7 +57,9 @@ size_t CrossRoutingContextReader::GetIndexInAdjMatrix(IngoingEdgeIteratorT ingoi void CrossRoutingContextReader::Load(Reader const & r) { - size_t size, pos = 0; + size_t pos = 0; + + uint32_t size; r.Read(pos, &size, sizeof(size)); pos += sizeof(size); m_ingoingNodes.resize(size); @@ -81,7 +83,6 @@ void CrossRoutingContextReader::Load(Reader const & r) pos += sizeof(strsize); for (uint32_t i = 0; i < strsize; ++i) { - vector tmpString; r.Read(pos, &size, sizeof(size)); pos += sizeof(size); vector buffer(size); From be305b23f0f85d53473da30e80d460adaeadad7a Mon Sep 17 00:00:00 2001 From: vng Date: Fri, 25 Sep 2015 18:23:26 +0300 Subject: [PATCH 5/5] Review fixes. --- android/jni/com/mapswithme/core/logging.cpp | 2 +- base/logging.cpp | 6 ++---- indexer/index.hpp | 4 +--- indexer/mwm_set.hpp | 6 ++---- map/feature_vec_model.hpp | 2 +- map/map_tests/mwm_set_test.cpp | 19 ++++++++++++++----- 6 files changed, 21 insertions(+), 18 deletions(-) diff --git a/android/jni/com/mapswithme/core/logging.cpp b/android/jni/com/mapswithme/core/logging.cpp index bcda11d230..58d488e1e0 100644 --- a/android/jni/com/mapswithme/core/logging.cpp +++ b/android/jni/com/mapswithme/core/logging.cpp @@ -38,7 +38,7 @@ void AndroidMessage(LogLevel level, SrcPoint const & src, string const & s) void AndroidLogMessage(LogLevel level, SrcPoint const & src, string const & s) { AndroidMessage(level, src, s); - CHECK(level < g_LogAbortLevel, ("Abort. Log level is too serious", level)); + CHECK_LESS(level, g_LogAbortLevel, ("Abort. Log level is too serious", level)); } void AndroidAssertMessage(SrcPoint const & src, string const & s) diff --git a/base/logging.cpp b/base/logging.cpp index 0ac3b05cac..713903ae6a 100644 --- a/base/logging.cpp +++ b/base/logging.cpp @@ -60,7 +60,6 @@ namespace my void LogMessageDefault(LogLevel level, SrcPoint const & srcPoint, string const & msg) { lock_guard lock(g_logMutex); - UNUSED_VALUE(lock); static LogHelper logger; @@ -70,19 +69,18 @@ namespace my out << DebugPrint(srcPoint) << msg << endl; std::cerr << out.str(); - CHECK(level < g_LogAbortLevel, ("Abort. Log level is too serious", level)); + CHECK_LESS(level, g_LogAbortLevel, ("Abort. Log level is too serious", level)); } void LogMessageTests(LogLevel level, SrcPoint const &, string const & msg) { lock_guard lock(g_logMutex); - UNUSED_VALUE(lock); ostringstream out; out << msg << endl; std::cerr << out.str(); - CHECK(level < g_LogAbortLevel, ("Abort. Log level is too serious", level)); + CHECK_LESS(level, g_LogAbortLevel, ("Abort. Log level is too serious", level)); } LogMessageFn LogMessage = &LogMessageDefault; diff --git a/indexer/index.hpp b/indexer/index.hpp index eab3b90c79..02ab500383 100644 --- a/indexer/index.hpp +++ b/indexer/index.hpp @@ -73,10 +73,8 @@ public: virtual void OnMapDeregistered(platform::LocalCountryFile const & localFile) {} }; - /// Registers a new map. - WARN_UNUSED_RESULT pair RegisterMap( - platform::LocalCountryFile const & localFile); + pair RegisterMap(platform::LocalCountryFile const & localFile); /// Deregisters a map from internal records. /// diff --git a/indexer/mwm_set.hpp b/indexer/mwm_set.hpp index fb847e5f10..652d4be163 100644 --- a/indexer/mwm_set.hpp +++ b/indexer/mwm_set.hpp @@ -164,12 +164,10 @@ public: /// are older than the localFile (in this case mwm handle will point /// to just-registered file). protected: - WARN_UNUSED_RESULT pair RegisterImpl( - platform::LocalCountryFile const & localFile); + pair RegisterImpl(platform::LocalCountryFile const & localFile); public: - WARN_UNUSED_RESULT pair Register( - platform::LocalCountryFile const & localFile); + pair Register(platform::LocalCountryFile const & localFile); //@} /// @name Remove mwm. diff --git a/map/feature_vec_model.hpp b/map/feature_vec_model.hpp index fce5e8b0e4..659a263c17 100644 --- a/map/feature_vec_model.hpp +++ b/map/feature_vec_model.hpp @@ -46,7 +46,7 @@ class FeaturesFetcher : public Index::Observer } /// Registers a new map. - WARN_UNUSED_RESULT pair RegisterMap( + pair RegisterMap( platform::LocalCountryFile const & localFile); /// Deregisters a map denoted by file from internal records. diff --git a/map/map_tests/mwm_set_test.cpp b/map/map_tests/mwm_set_test.cpp index 3ea80b0536..d3919c19c8 100644 --- a/map/map_tests/mwm_set_test.cpp +++ b/map/map_tests/mwm_set_test.cpp @@ -5,6 +5,8 @@ #include "platform/local_country_file_utils.hpp" #include "platform/platform.hpp" +#include "base/scope_guard.hpp" + #ifndef OMIM_OS_WINDOWS #include #endif @@ -22,6 +24,7 @@ UNIT_TEST(MwmSet_FileSystemErrors) LocalCountryFile localFile(dir, file, 0); TEST(CountryIndexes::DeleteFromDisk(localFile), ()); + // Maximum level to check exception handling logic. LogLevel oldLevel = g_LogAbortLevel; g_LogAbortLevel = LCRITICAL; @@ -29,21 +32,27 @@ UNIT_TEST(MwmSet_FileSystemErrors) int const readOnlyMode = S_IRUSR | S_IRGRP | S_IROTH | S_IXUSR | S_IXGRP | S_IXOTH; TEST_EQUAL(chmod(dir.c_str(), readOnlyMode), 0, ()); + auto restoreFn = [oldLevel, &dir, readOnlyMode] () + { + g_LogAbortLevel = oldLevel; + TEST_EQUAL(chmod(dir.c_str(), readOnlyMode | S_IWUSR), 0, ()); + }; + MY_SCOPE_GUARD(restoreGuard, restoreFn); + Index index; auto p = index.RegisterMap(localFile); TEST_EQUAL(p.second, Index::RegResult::Success, ()); + // Registering should pass ok. TEST(index.GetMwmIdByCountryFile(file) != Index::MwmId(), ()); + // Getting handle causes feature offsets index building which should fail + // because of write permissions. TEST(!index.GetMwmHandleById(p.first).IsAlive(), ()); + // Map is automatically deregistered after the fail. vector> infos; index.GetMwmsInfo(infos); TEST(infos.empty(), ()); - - // Restore writable permission. - TEST_EQUAL(chmod(dir.c_str(), readOnlyMode | S_IWUSR), 0, ()); - - g_LogAbortLevel = oldLevel; } #endif