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.
Project-OSRM · Jan 19, 2016 · e21eaa4 · e21eaa4
1 parent 80b897d
commit e21eaa4
Show file tree

Hide file tree

Showing 2 changed files with 70 additions and 39 deletions.
diff --git a/include/engine/datafacade/shared_datafacade.hpp b/include/engine/datafacade/shared_datafacade.hpp
@@ -223,6 +223,8 @@ template <class EdgeDataT> class SharedDataFacade final : public BaseDataFacade<
   public:
     virtual ~SharedDataFacade() {}
 
+    boost::shared_mutex data_mutex;
+
     SharedDataFacade()
     {
         if (!datastore::SharedMemory::RegionExists(CURRENT_REGIONS))
@@ -243,57 +245,76 @@ template <class EdgeDataT> 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
-            datastore::SharedMemory::Remove(CURRENT_LAYOUT);
-            datastore::SharedMemory::Remove(CURRENT_DATA);
-
-            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(datastore::SharedMemoryFactory::Get(CURRENT_LAYOUT));
+            // Get exclusive lock
+            util::SimpleLogger().Write(logDEBUG) << "Updates available, getting exclusive lock";
+            boost::unique_lock<boost::shared_mutex> lock(data_mutex);
 
-            data_layout = (SharedDataLayout *)(m_layout_memory->Ptr());
+            if (CURRENT_LAYOUT != data_timestamp_ptr->layout ||
+                CURRENT_DATA != data_timestamp_ptr->data)
+            {
+                // release the previous shared memory segments
+                datastore::SharedMemory::Remove(CURRENT_LAYOUT);
+                datastore::SharedMemory::Remove(CURRENT_DATA);
 
-            m_large_memory.reset(datastore::SharedMemoryFactory::Get(CURRENT_DATA));
-            shared_memory = (char *)(m_large_memory->Ptr());
+                CURRENT_LAYOUT = data_timestamp_ptr->layout;
+                CURRENT_DATA = data_timestamp_ptr->data;
+                CURRENT_TIMESTAMP = 0;  // Force trigger a reload
 
-            const char *file_index_ptr =
-                data_layout->GetBlockPtr<char>(shared_memory, SharedDataLayout::FILE_INDEX_PATH);
-            file_index_path = boost::filesystem::path(file_index_ptr);
-            if (!boost::filesystem::exists(file_index_path))
+                util::SimpleLogger().Write(logDEBUG) << "Current layout was different to new layout, swapping";
+            }
+            else
             {
-                util::SimpleLogger().Write(logDEBUG) << "Leaf file name "
-                                                     << file_index_path.string();
-                throw util::exception("Could not load leaf index file. "
-                                      "Is any data loaded into shared memory?");
+                util::SimpleLogger().Write(logDEBUG) << "Current layout was same to new layout, not swapping";
+
             }
 
-            LoadGraph();
-            LoadChecksum();
-            LoadNodeAndEdgeInformation();
-            LoadGeometries();
-            LoadTimestamp();
-            LoadViaNodeList();
-            LoadNames();
-            LoadCoreInformation();
+            if (CURRENT_TIMESTAMP != data_timestamp_ptr->timestamp)
+            {
+                CURRENT_TIMESTAMP = data_timestamp_ptr->timestamp;
 
-            data_layout->PrintInformation();
+                util::SimpleLogger().Write(logDEBUG) << "Performing data reload";
+                m_layout_memory.reset(datastore::SharedMemoryFactory::Get(CURRENT_LAYOUT));
 
-            util::SimpleLogger().Write() << "number of geometries: " << m_coordinate_list->size();
-            for (unsigned i = 0; i < m_coordinate_list->size(); ++i)
-            {
-                if (!GetCoordinateOfNode(i).IsValid())
+                data_layout = (SharedDataLayout *) (m_layout_memory->Ptr());
+
+                m_large_memory.reset(datastore::SharedMemoryFactory::Get(CURRENT_DATA));
+                shared_memory = (char *) (m_large_memory->Ptr());
+
+                const char *file_index_ptr =
+                        data_layout->GetBlockPtr<char>(shared_memory, SharedDataLayout::FILE_INDEX_PATH);
+                file_index_path = boost::filesystem::path(file_index_ptr);
+                if (!boost::filesystem::exists(file_index_path)) {
+                    util::SimpleLogger().Write(logDEBUG) << "Leaf file name "
+                    << file_index_path.string();
+                    throw util::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();
+
+                util::SimpleLogger().Write() << "number of geometries: " << m_coordinate_list->size();
+                for (unsigned i = 0; i < m_coordinate_list->size(); ++i)
                 {
-                    util::SimpleLogger().Write() << "coordinate " << i << " not valid";
+                    if (!GetCoordinateOfNode(i).IsValid())
+                    {
+                        util::SimpleLogger().Write() << "coordinate " << i << " not valid";
+                    }
                 }
             }
+            util::SimpleLogger().Write(logDEBUG) << "Releasing exclusive lock";
         }
     }
 

diff --git a/src/engine/osrm_impl.cpp b/src/engine/osrm_impl.cpp
@@ -82,8 +82,18 @@ int OSRM::OSRM_impl::RunQuery(const RouteParameters &route_parameters,
         return 400;
     }
 
+    osrm::engine::plugins::BasePlugin::Status return_code;
     increase_concurrent_query_count();
-    auto return_code = plugin_iterator->second->HandleRequest(route_parameters, json_result);
+    if (barrier) {
+        // Get a shared data lock so that other threads won't update
+        // things while the query is running
+        boost::shared_lock<boost::shared_mutex> data_lock{
+                    (static_cast<datafacade::SharedDataFacade<contractor::QueryEdge::EdgeData> *>(
+                            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<int>(return_code);
 }