Update changelog

This fixes issue #3016.

CHANGELOG.md

@@ -1,3 +1,35 @@
+# 5.4.1
+  - Changes from 5.4.0
+    - Bugfixes
+      - #3016: Fixes shared memory updates while queries are running
+
+# 5.4.0
+  - Changes from 5.3.0
+    - Profiles
+      - includes library guidance.lua that offers preliminary configuration on guidance.
+      - added left_hand_driving flag in global profile properties
+      - modified turn penalty function for car profile - better fit to real data
+      - return `ref` and `name` as separate fields. Do no use ref or destination as fallback for name value
+    - Guidance
+      - Handle Access tags for lanes, only considering valid lanes in lane-guidance (think car | car | bike | car)
+    - API:
+      - `annotations=true` now returns the data source id for each segment as `datasources`
+      - Reduced semantic of merge to refer only to merges from a lane onto a motorway-like road
+      - new `ref` field in the `RouteStep` object. It contains the reference code or name of a way. Previously merged into the `name` property like `name (ref)` and are now separate fields.
+    - Bugfixes
+      - Fixed an issue that would result in segfaults for viaroutes with an invalid intermediate segment when u-turns were allowed at the via-location
+      - Invalid only_* restrictions could result in loss of connectivity. As a fallback, we assume all turns allowed when the restriction is not valid
+      - Fixed a bug that could result in an infinite loop when finding information about an upcoming intersection
+      - Fixed a bug that led to not discovering if a road simply looses a considered prefix
+      - BREAKING: Fixed a bug that could crash postprocessing of instructions on invalid roundabout taggings. This change requires reprocessing datasets with osrm-extract and osrm-contract
+      - Fixed an issue that could emit `invalid` as instruction when ending on a sliproad after a traffic-light
+      - Fixed an issue that would detect turning circles as sliproads
+      - Fixed a bug where post-processing instructions (e.g. left + left -> uturn) could result in false pronunciations
+      - Fixes a bug where a bearing range of zero would cause exhaustive graph traversals
+      - Fixes a bug where certain looped geometries could cause an infinite loop during extraction
+    - Infrastructure:
+      - Adds a feature to limit results in nearest service with a default of 100 in `osrm-routed`
+
 # 5.4.0-rc.7
   - Chages from 5.4.0-rc.6
     - Bugfixes re-introduce space between two entries in summaries
@@ -33,34 +65,6 @@
     - Trip Plugin
       - changed internal behaviour to prefer the smallest lexicographic result over the largest one
 
-# 5.4.0
-  - Changes from 5.3.0
-    - Profiles
-      - includes library guidance.lua that offers preliminary configuration on guidance.
-      - added left_hand_driving flag in global profile properties
-      - modified turn penalty function for car profile - better fit to real data
-      - return `ref` and `name` as separate fields. Do no use ref or destination as fallback for name value
-    - Guidance
-      - Handle Access tags for lanes, only considering valid lanes in lane-guidance (think car | car | bike | car)
-    - API:
-      - `annotations=true` now returns the data source id for each segment as `datasources`
-      - Reduced semantic of merge to refer only to merges from a lane onto a motorway-like road
-      - new `ref` field in the `RouteStep` object. It contains the reference code or name of a way. Previously merged into the `name` property like `name (ref)` and are now separate fields.
-    - Bugfixes
-      - Fixed an issue that would result in segfaults for viaroutes with an invalid intermediate segment when u-turns were allowed at the via-location
-      - Invalid only_* restrictions could result in loss of connectivity. As a fallback, we assume all turns allowed when the restriction is not valid
-      - Fixed a bug that could result in an infinite loop when finding information about an upcoming intersection
-      - Fixed a bug that led to not discovering if a road simply looses a considered prefix
-      - BREAKING: Fixed a bug that could crash postprocessing of instructions on invalid roundabout taggings. This change requires reprocessing datasets with osrm-extract and osrm-contract
-      - Fixed an issue that could emit `invalid` as instruction when ending on a sliproad after a traffic-light
-      - Fixed an issue that would detect turning circles as sliproads
-      - Fixed a bug where post-processing instructions (e.g. left + left -> uturn) could result in false pronunciations
-      - Fixes a bug where a bearing range of zero would cause exhaustive graph traversals
-      - Fixes a bug where certain looped geometries could cause an infinite loop during extraction
-
-    - Infrastructure:
-      - Adds a feature to limit results in nearest service with a default of 100 in `osrm-routed`
-
 # 5.3.0
   - Changes from 5.3.0-rc.3
     - Guidance

include/engine/engine.hpp

@@ -1,7 +1,6 @@
 #ifndef ENGINE_HPP
 #define ENGINE_HPP
 
-#include "storage/shared_barriers.hpp"
 #include "engine/status.hpp"
 #include "util/json_container.hpp"
 
@@ -20,6 +19,11 @@ struct Object;
 }
 }
 
+namespace storage
+{
+struct SharedBarriers;
+}
+
 // Fwd decls
 namespace engine
 {
@@ -52,9 +56,6 @@ class BaseDataFacade;
 class Engine final
 {
   public:
-    // Needs to be public
-    struct EngineLock;
-
     explicit Engine(const EngineConfig &config);
 
     Engine(Engine &&) noexcept;
@@ -71,7 +72,7 @@ class Engine final
     Status Tile(const api::TileParameters &parameters, std::string &result) const;
 
   private:
-    std::unique_ptr<EngineLock> lock;
+    std::unique_ptr<storage::SharedBarriers> lock;
 
     std::unique_ptr<plugins::ViaRoutePlugin> route_plugin;
     std::unique_ptr<plugins::TablePlugin> table_plugin;

include/storage/shared_barriers.hpp

@@ -3,6 +3,7 @@
 
 #include <boost/interprocess/sync/named_condition.hpp>
 #include <boost/interprocess/sync/named_mutex.hpp>
+#include <boost/interprocess/sync/named_sharable_mutex.hpp>
 
 namespace osrm
 {
@@ -13,25 +14,13 @@ struct SharedBarriers
 
     SharedBarriers()
         : pending_update_mutex(boost::interprocess::open_or_create, "pending_update"),
-          update_mutex(boost::interprocess::open_or_create, "update"),
+          query_mutex(boost::interprocess::open_or_create, "query")
-          query_mutex(boost::interprocess::open_or_create, "query"),
-          no_running_queries_condition(boost::interprocess::open_or_create, "no_running_queries"),
-          update_ongoing(false), number_of_queries(0)
     {
     }
 
     // Mutex to protect access to the boolean variable
     boost::interprocess::named_mutex pending_update_mutex;
-    boost::interprocess::named_mutex update_mutex;
+    boost::interprocess::named_sharable_mutex query_mutex;
-    boost::interprocess::named_mutex query_mutex;
-
-    // Condition that no update is running
-    boost::interprocess::named_condition no_running_queries_condition;
-
-    // Is there an ongoing update?
-    bool update_ongoing;
-    // Is there any query?
-    int number_of_queries;
 };
 }
 }

src/engine/engine.cpp

@@ -1,5 +1,5 @@
-#include "engine/engine.hpp"
 #include "engine/api/route_parameters.hpp"
+#include "engine/engine.hpp"
 #include "engine/engine_config.hpp"
 #include "engine/status.hpp"
 
@@ -21,6 +21,7 @@
 #include <boost/assert.hpp>
 #include <boost/interprocess/sync/named_condition.hpp>
 #include <boost/interprocess/sync/scoped_lock.hpp>
+#include <boost/interprocess/sync/sharable_lock.hpp>
 #include <boost/thread/lock_types.hpp>
 
 #include <algorithm>
@@ -28,68 +29,17 @@
 #include <utility>
 #include <vector>
 
-namespace osrm
-{
-namespace engine
-{
-struct Engine::EngineLock
-{
-    // will only be initialized if shared memory is used
-    storage::SharedBarriers barrier;
-    // decrease number of concurrent queries
-    void DecreaseQueryCount();
-    // increase number of concurrent queries
-    void IncreaseQueryCount();
-};
-
-// decrease number of concurrent queries
-void Engine::EngineLock::DecreaseQueryCount()
-{
-    // lock query
-    boost::interprocess::scoped_lock<boost::interprocess::named_mutex> query_lock(
-        barrier.query_mutex);
-
-    // decrement query count
-    --(barrier.number_of_queries);
-    BOOST_ASSERT_MSG(0 <= barrier.number_of_queries, "invalid number of queries");
-
-    // notify all processes that were waiting for this condition
-    if (0 == barrier.number_of_queries)
-    {
-        barrier.no_running_queries_condition.notify_all();
-    }
-}
-
-// increase number of concurrent queries
-void Engine::EngineLock::IncreaseQueryCount()
-{
-    // lock update pending
-    boost::interprocess::scoped_lock<boost::interprocess::named_mutex> pending_lock(
-        barrier.pending_update_mutex);
-
-    // lock query
-    boost::interprocess::scoped_lock<boost::interprocess::named_mutex> query_lock(
-        barrier.query_mutex);
-
-    // unlock update pending
-    pending_lock.unlock();
-
-    // increment query count
-    ++(barrier.number_of_queries);
-}
-} // ns engine
-} // ns osrm
-
 namespace
 {
 // Abstracted away the query locking into a template function
 // Works the same for every plugin.
 template <typename ParameterT, typename PluginT, typename ResultT>
-osrm::engine::Status RunQuery(const std::unique_ptr<osrm::engine::Engine::EngineLock> &lock,
+osrm::engine::Status
-                              osrm::engine::datafacade::BaseDataFacade &facade,
+RunQuery(const std::unique_ptr<osrm::storage::SharedBarriers> &lock,
-                              const ParameterT &parameters,
+         osrm::engine::datafacade::BaseDataFacade &facade,
-                              PluginT &plugin,
+         const ParameterT &parameters,
-                              ResultT &result)
+         PluginT &plugin,
+         ResultT &result)
 {
     if (!lock)
     {
@@ -97,7 +47,10 @@ osrm::engine::Status RunQuery(const std::unique_ptr<osrm::engine::Engine::Engine
     }
 
     BOOST_ASSERT(lock);
-    lock->IncreaseQueryCount();
+    // this locks aquires shared ownership of the query mutex: other requets are allowed
+    // to run, but data updates need to wait for all queries to finish until they can aquire an exclusive lock
+    boost::interprocess::sharable_lock<boost::interprocess::named_sharable_mutex> query_lock(
+        lock->query_mutex);
 
     auto &shared_facade = static_cast<osrm::engine::datafacade::SharedDataFacade &>(facade);
     shared_facade.CheckAndReloadFacade();
@@ -107,7 +60,6 @@ osrm::engine::Status RunQuery(const std::unique_ptr<osrm::engine::Engine::Engine
 
     osrm::engine::Status status = plugin.HandleRequest(parameters, result);
 
-    lock->DecreaseQueryCount();
     return status;
 }
 
@@ -125,10 +77,11 @@ namespace engine
 {
 
 Engine::Engine(const EngineConfig &config)
+    : lock(config.use_shared_memory ? std::make_unique<storage::SharedBarriers>()
+                                    : std::unique_ptr<storage::SharedBarriers>())
 {
     if (config.use_shared_memory)
     {
-        lock = util::make_unique<EngineLock>();
         query_data_facade = util::make_unique<datafacade::SharedDataFacade>();
     }
     else

src/storage/storage.cpp

@@ -42,11 +42,6 @@ namespace osrm
 namespace storage
 {
 
-using RTreeLeaf = engine::datafacade::BaseDataFacade::RTreeLeaf;
-using RTreeNode =
-    util::StaticRTree<RTreeLeaf, util::ShM<util::Coordinate, true>::vector, true>::TreeNode;
-using QueryGraph = util::StaticGraph<contractor::QueryEdge::EdgeData>;
-
 // delete a shared memory region. report warning if it could not be deleted
 void deleteRegion(const SharedDataType region)
 {
@@ -76,6 +71,11 @@ void deleteRegion(const SharedDataType region)
     }
 }
 
+using RTreeLeaf = engine::datafacade::BaseDataFacade::RTreeLeaf;
+using RTreeNode =
+    util::StaticRTree<RTreeLeaf, util::ShM<util::Coordinate, true>::vector, true>::TreeNode;
+using QueryGraph = util::StaticGraph<contractor::QueryEdge::EdgeData>;
+
 Storage::Storage(StorageConfig config_) : config(std::move(config_)) {}
 
 int Storage::Run()
@@ -738,20 +738,15 @@ int Storage::Run()
     SharedDataTimestamp *data_timestamp_ptr =
         static_cast<SharedDataTimestamp *>(data_type_memory->Ptr());
 
-    boost::interprocess::scoped_lock<boost::interprocess::named_mutex> query_lock(
-        barrier.query_mutex);
-
-    // notify all processes that were waiting for this condition
-    if (0 < barrier.number_of_queries)
     {
-        barrier.no_running_queries_condition.wait(query_lock);
+        boost::interprocess::scoped_lock<boost::interprocess::named_sharable_mutex> query_lock(barrier.query_mutex);
-    }
 
-    data_timestamp_ptr->layout = layout_region;
+        data_timestamp_ptr->layout = layout_region;
-    data_timestamp_ptr->data = data_region;
+        data_timestamp_ptr->data = data_region;
-    data_timestamp_ptr->timestamp += 1;
+        data_timestamp_ptr->timestamp += 1;
-    deleteRegion(previous_data_region);
+        deleteRegion(previous_data_region);
-    deleteRegion(previous_layout_region);
+        deleteRegion(previous_layout_region);
+    }
     util::SimpleLogger().Write() << "all data loaded";
 
     return EXIT_SUCCESS;

src/tools/unlock_all_mutexes.cpp

@@ -10,6 +10,5 @@ int main()
     osrm::storage::SharedBarriers barrier;
     barrier.pending_update_mutex.unlock();
     barrier.query_mutex.unlock();
-    barrier.update_mutex.unlock();
     return 0;
 }