From 23b2154d98c90b0cd7eec88c36f146d63114f216 Mon Sep 17 00:00:00 2001 From: Daniel Patterson Date: Fri, 15 Jan 2016 00:57:36 -0800 Subject: [PATCH] Adds a shared/exclusive lock around queries and CheckAndReloadFacade. Without this, it's possible for CheckAndReloadFacade to start working while a query is still in progress, leading to undefined behaviour. --- library/osrm_impl.cpp | 11 +- server/data_structures/shared_datafacade.hpp | 102 +++++++++++-------- 2 files changed, 71 insertions(+), 42 deletions(-) diff --git a/library/osrm_impl.cpp b/library/osrm_impl.cpp index 3f5b62b19..069ef95a4 100644 --- a/library/osrm_impl.cpp +++ b/library/osrm_impl.cpp @@ -109,7 +109,16 @@ int OSRM::OSRM_impl::RunQuery(const RouteParameters &route_parameters, osrm::jso } increase_concurrent_query_count(); - auto return_code = plugin_iterator->second->HandleRequest(route_parameters, json_result); + BasePlugin::Status return_code; + if (barrier) { + // Get a shared data lock so that other threads won't update + // things while the query is running + boost::shared_lock data_lock{ + (static_cast *>(query_data_facade))->data_mutex}; + return_code = plugin_iterator->second->HandleRequest(route_parameters, json_result); + } else { + return_code = plugin_iterator->second->HandleRequest(route_parameters, json_result); + } decrease_concurrent_query_count(); return static_cast(return_code); } diff --git a/server/data_structures/shared_datafacade.hpp b/server/data_structures/shared_datafacade.hpp index 1c71d043f..4bbf076d3 100644 --- a/server/data_structures/shared_datafacade.hpp +++ b/server/data_structures/shared_datafacade.hpp @@ -239,6 +239,8 @@ template class SharedDataFacade final : public BaseDataFacade< public: virtual ~SharedDataFacade() {} + boost::shared_mutex data_mutex; + SharedDataFacade() { if (!SharedMemory::RegionExists(CURRENT_REGIONS)) @@ -259,56 +261,74 @@ template class SharedDataFacade final : public BaseDataFacade< void CheckAndReloadFacade() { if (CURRENT_LAYOUT != data_timestamp_ptr->layout || - CURRENT_DATA != data_timestamp_ptr->data) + CURRENT_DATA != data_timestamp_ptr->data || + CURRENT_TIMESTAMP != data_timestamp_ptr->timestamp) { - // release the previous shared memory segments - SharedMemory::Remove(CURRENT_LAYOUT); - SharedMemory::Remove(CURRENT_DATA); + // Get exclusive lock + SimpleLogger().Write(logDEBUG) << "Updates available, getting exclusive lock"; + boost::unique_lock lock(data_mutex); - CURRENT_LAYOUT = data_timestamp_ptr->layout; - CURRENT_DATA = data_timestamp_ptr->data; - CURRENT_TIMESTAMP = 0; // Force trigger a reload - } - - if (CURRENT_TIMESTAMP != data_timestamp_ptr->timestamp) - { - CURRENT_TIMESTAMP = data_timestamp_ptr->timestamp; - m_layout_memory.reset(SharedMemoryFactory::Get(CURRENT_LAYOUT)); - - data_layout = (SharedDataLayout *)(m_layout_memory->Ptr()); - - m_large_memory.reset(SharedMemoryFactory::Get(CURRENT_DATA)); - shared_memory = (char *)(m_large_memory->Ptr()); - - const char *file_index_ptr = - data_layout->GetBlockPtr(shared_memory, SharedDataLayout::FILE_INDEX_PATH); - file_index_path = boost::filesystem::path(file_index_ptr); - if (!boost::filesystem::exists(file_index_path)) + if (CURRENT_LAYOUT != data_timestamp_ptr->layout || + CURRENT_DATA != data_timestamp_ptr->data) { - SimpleLogger().Write(logDEBUG) << "Leaf file name " << file_index_path.string(); - throw osrm::exception("Could not load leaf index file. " - "Is any data loaded into shared memory?"); + // release the previous shared memory segments + SharedMemory::Remove(CURRENT_LAYOUT); + SharedMemory::Remove(CURRENT_DATA); + + CURRENT_LAYOUT = data_timestamp_ptr->layout; + CURRENT_DATA = data_timestamp_ptr->data; + CURRENT_TIMESTAMP = 0; // Force trigger a reload + + SimpleLogger().Write(logDEBUG) << "Current layout was different to new layout, swapping"; + } + else + { + SimpleLogger().Write(logDEBUG) << "Current layout was same to new layout, not swapping"; } - LoadGraph(); - LoadChecksum(); - LoadNodeAndEdgeInformation(); - LoadGeometries(); - LoadTimestamp(); - LoadViaNodeList(); - LoadNames(); - LoadCoreInformation(); - - data_layout->PrintInformation(); - - SimpleLogger().Write() << "number of geometries: " << m_coordinate_list->size(); - for (unsigned i = 0; i < m_coordinate_list->size(); ++i) + if (CURRENT_TIMESTAMP != data_timestamp_ptr->timestamp) { - if (!GetCoordinateOfNode(i).is_valid()) + CURRENT_TIMESTAMP = data_timestamp_ptr->timestamp; + + SimpleLogger().Write(logDEBUG) << "Performing data reload"; + m_layout_memory.reset(SharedMemoryFactory::Get(CURRENT_LAYOUT)); + + data_layout = (SharedDataLayout *) (m_layout_memory->Ptr()); + + m_large_memory.reset(SharedMemoryFactory::Get(CURRENT_DATA)); + shared_memory = (char *) (m_large_memory->Ptr()); + + const char *file_index_ptr = + data_layout->GetBlockPtr(shared_memory, SharedDataLayout::FILE_INDEX_PATH); + file_index_path = boost::filesystem::path(file_index_ptr); + if (!boost::filesystem::exists(file_index_path)) { + SimpleLogger().Write(logDEBUG) << "Leaf file name " + << file_index_path.string(); + throw osrm::exception("Could not load leaf index file. " + "Is any data loaded into shared memory?"); + } + + LoadGraph(); + LoadChecksum(); + LoadNodeAndEdgeInformation(); + LoadGeometries(); + LoadTimestamp(); + LoadViaNodeList(); + LoadNames(); + LoadCoreInformation(); + + data_layout->PrintInformation(); + + SimpleLogger().Write() << "number of geometries: " << m_coordinate_list->size(); + for (unsigned i = 0; i < m_coordinate_list->size(); ++i) { - SimpleLogger().Write() << "coordinate " << i << " not valid"; + if (!GetCoordinateOfNode(i).is_valid()) + { + SimpleLogger().Write() << "coordinate " << i << " not valid"; + } } } + SimpleLogger().Write(logDEBUG) << "Releasing exclusive lock"; } }