Bump version field and update CHANGELOG.

This change fixes two bugs:

1. A dead-lock that occurs between osrm-datastore and libosrm when an
   old dataset is free during a data update. This happened because the
   mutexes where acquired in a different order.

2. A region is deleted eventhough it is still in use. This happens when
   libosrm gets overtaken by osrm-datastore, so the new dataset is in
   the same region the old one was.

CHANGELOG.md

@@ -1,3 +1,8 @@
+# 5.5.1
+  - Changes from 5.5.0
+    - Bugfixes
+      - Fixes #3455 where a deadlock could occur if re-loading new data under heavy load with multiple consumers osrm-datastore
+
 # 5.5.0
   - Changes from 5.4.0
     - API:
@@ -29,6 +34,7 @@
       - fixed a bug that could result in crashes when leaving a ferry directly onto a motorway ramp
       - fixed a bug in the tile plugin that resulted in discovering invalid edges for connections
       - improved error messages when missing files during traffic updates (#3114)
+      - For single coordinate geometries the GeoJSON `Point` encoding was broken. We now always emit `LineString`s even in the one-coordinate-case (backwards compatible) (#3425)
     - Debug Tiles
       - Added support for turn penalties
     - Internals

CMakeLists.txt

@@ -53,7 +53,7 @@ endif()
 project(OSRM C CXX)
 set(OSRM_VERSION_MAJOR 5)
 set(OSRM_VERSION_MINOR 5)
-set(OSRM_VERSION_PATCH 0)
+set(OSRM_VERSION_PATCH 1)
 set(OSRM_VERSION "${OSRM_VERSION_MAJOR}.${OSRM_VERSION_MINOR}.${OSRM_VERSION_PATCH}")
 
 add_definitions(-DOSRM_PROJECT_DIR="${CMAKE_CURRENT_SOURCE_DIR}")

docs/http.md

@@ -519,7 +519,7 @@ step.
 |------------|--------------------------------------------------------------------|
 | polyline   | [polyline](https://www.npmjs.com/package/polyline) with precision 5 in [latitude,longitude] encoding |
 | polyline6  | [polyline](https://www.npmjs.com/package/polyline) with precision 6 in [latitude,longitude] encoding |
-| geojson    | [GeoJSON `LineString`](http://geojson.org/geojson-spec.html#linestring) or [GeoJSON `Point`](http://geojson.org/geojson-spec.html#point) if it is only one coordinate (not wrapped by a GeoJSON feature)|
+| geojson    | [GeoJSON `LineString`](http://geojson.org/geojson-spec.html#linestring) |
 
 - `name`: The name of the way along which travel proceeds.
 - `ref`: A reference number or code for the way. Optionally included, if ref data is available for the given way.

features/car/access.feature

@@ -220,3 +220,15 @@ Feature: Car - Restricted access
         Then routability should be
             | highway | toll        | bothw |
             | primary | yes         |       |
+
+    Scenario: Car - directional access tags
+        Then routability should be
+            | highway | access | access:forward | access:backward | forw | backw |
+            | primary | yes    | yes            | yes             | x    | x     |
+            | primary | yes    |                | no              | x    |       |
+            | primary | yes    | no             |                 |      | x     |
+            | primary | yes    | no             | no              |      |       |
+            | primary | no     | no             | no              |      |       |
+            | primary | no     |                | yes             |      | x     |
+            | primary | no     | yes            |                 | x    |       |
+            | primary | no     | yes            | yes             | x    | x     |

include/contractor/contractor.hpp

@@ -81,6 +81,7 @@ class Contractor
     EdgeID
     LoadEdgeExpandedGraph(const std::string &edge_based_graph_path,
                           util::DeallocatingVector<extractor::EdgeBasedEdge> &edge_based_edge_list,
+                          std::vector<EdgeWeight> &node_weights,
                           const std::string &edge_segment_lookup_path,
                           const std::string &edge_penalty_path,
                           const std::vector<std::string> &segment_speed_path,

include/engine/api/json_factory.hpp

@@ -54,22 +54,25 @@ util::json::Object makeGeoJSONGeometry(ForwardIter begin, ForwardIter end)
     auto num_coordinates = std::distance(begin, end);
     BOOST_ASSERT(num_coordinates != 0);
     util::json::Object geojson;
+    geojson.values["type"] = "LineString";
+    util::json::Array coordinates;
     if (num_coordinates > 1)
     {
-        geojson.values["type"] = "LineString";
-        util::json::Array coordinates;
         coordinates.values.reserve(num_coordinates);
-        std::transform(
-            begin, end, std::back_inserter(coordinates.values), &detail::coordinateToLonLat);
-        geojson.values["coordinates"] = std::move(coordinates);
+        auto into = std::back_inserter(coordinates.values);
+        std::transform(begin, end, into, &detail::coordinateToLonLat);
     }
     else if (num_coordinates > 0)
     {
-        geojson.values["type"] = "Point";
-        util::json::Array coordinates;
-        coordinates.values.push_back(detail::coordinateToLonLat(*begin));
-        geojson.values["coordinates"] = std::move(coordinates);
+        // For a single location we create a [location, location] LineString
+        // instead of a single Point making the GeoJSON output consistent.
+        coordinates.values.reserve(2);
+        auto location = detail::coordinateToLonLat(*begin);
+        coordinates.values.push_back(location);
+        coordinates.values.push_back(location);
     }
+    geojson.values["coordinates"] = std::move(coordinates);
+
     return geojson;
 }
 

include/engine/datafacade/shared_memory_datafacade.hpp

@@ -38,30 +38,39 @@ class SharedMemoryDataFacade : public ContiguousInternalMemoryDataFacadeBase
     // used anymore.  We crash hard here if something goes wrong (noexcept).
     virtual ~SharedMemoryDataFacade() noexcept
     {
+        // Now check if this is still the newest dataset
+        boost::interprocess::sharable_lock<boost::interprocess::named_upgradable_mutex>
+            current_regions_lock(shared_barriers->current_regions_mutex,
+                                 boost::interprocess::defer_lock);
+
         boost::interprocess::scoped_lock<boost::interprocess::named_sharable_mutex> exclusive_lock(
             data_region == storage::DATA_1 ? shared_barriers->regions_1_mutex
                                            : shared_barriers->regions_2_mutex,
             boost::interprocess::defer_lock);
 
         // if this returns false this is still in use
-        if (exclusive_lock.try_lock())
+        if (current_regions_lock.try_lock() && exclusive_lock.try_lock())
         {
-            // Now check if this is still the newest dataset
-            const boost::interprocess::sharable_lock<boost::interprocess::named_upgradable_mutex>
-                lock(shared_barriers->current_regions_mutex);
-
-            auto shared_regions = storage::makeSharedMemory(storage::CURRENT_REGIONS);
-            const auto current_timestamp =
-                static_cast<const storage::SharedDataTimestamp *>(shared_regions->Ptr());
-
-            if (current_timestamp->timestamp == shared_timestamp)
+            if (storage::SharedMemory::RegionExists(data_region))
             {
-                util::Log(logDEBUG) << "Retaining data with shared timestamp " << shared_timestamp;
-            }
-            else
-            {
-                storage::SharedMemory::Remove(data_region);
-                storage::SharedMemory::Remove(layout_region);
+                BOOST_ASSERT(storage::SharedMemory::RegionExists(layout_region));
+
+                auto shared_regions = storage::makeSharedMemory(storage::CURRENT_REGIONS);
+                const auto current_timestamp =
+                    static_cast<const storage::SharedDataTimestamp *>(shared_regions->Ptr());
+
+                // check if the memory region referenced by this facade needs cleanup
+                if (current_timestamp->data == data_region)
+                {
+                    BOOST_ASSERT(current_timestamp->layout == layout_region);
+                    util::Log(logDEBUG) << "Retaining data with shared timestamp "
+                                        << shared_timestamp;
+                }
+                else
+                {
+                    storage::SharedMemory::Remove(data_region);
+                    storage::SharedMemory::Remove(layout_region);
+                }
             }
         }
     }

include/extractor/guidance/intersection_handler.hpp

@@ -122,6 +122,7 @@ template <typename IntersectionType> // works with Intersection and Intersection
 std::size_t IntersectionHandler::findObviousTurn(const EdgeID via_edge,
                                                  const IntersectionType &intersection) const
 {
+    using Road = typename IntersectionType::value_type;
     using EdgeData = osrm::util::NodeBasedDynamicGraph::EdgeData;
     using osrm::util::angularDeviation;
 
@@ -269,7 +270,7 @@ std::size_t IntersectionHandler::findObviousTurn(const EdgeID via_edge,
 
     // checks if continue candidates are sharp turns
     const bool all_continues_are_narrow = [&]() {
-        return std::count_if(begin(intersection), end(intersection), [&](const auto &road) {
+        return std::count_if(begin(intersection), end(intersection), [&](const Road &road) {
                    const EdgeData &road_data = node_based_graph.GetEdgeData(road.eid);
                    const double &road_angle = angularDeviation(road.angle, STRAIGHT_ANGLE);
                    return IsContinueRoad(road_data) && (road_angle < NARROW_TURN_ANGLE);

include/storage/io.hpp

@@ -161,7 +161,7 @@ class FileReader
         {
             std::getline(input_stream, thisline);
         }
-        catch (const std::ios_base::failure &e)
+        catch (const std::ios_base::failure & /*e*/)
         {
             // EOF is OK here, everything else, re-throw
             if (!input_stream.eof())

include/util/geojson_debug_logger.hpp

@@ -63,11 +63,11 @@ class GeojsonLogger
         // make sure to syncronize logging output, our writing should be sequential
         std::lock_guard<std::mutex> guard(lock);
 
-        // if there is no logfile, we cannot write
+        // if there is no logfile, we cannot write (possible reason: the guard might be out of scope
+        // (e.g. if it is anonymous))
         if (!ofs.is_open() || (nullptr == policy))
         {
-            // this can only happend between two guards when concurrent writing occurs
-            return false;
+            throw util::exception("Trying to use the geojson printer without an open logger.");
         }
 
         // use our policy to convert the arguments into geojson, this can be done in parallel

include/util/meminfo.hpp

@@ -0,0 +1,43 @@
+#ifndef MEMINFO_HPP
+#define MEMINFO_HPP
+
+#include "util/log.hpp"
+
+#include <stxxl/mng>
+#ifndef _WIN32
+#include <sys/resource.h>
+#endif
+
+namespace osrm
+{
+namespace util
+{
+inline void DumpMemoryStats()
+{
+#if STXXL_VERSION_MAJOR > 1 || (STXXL_VERSION_MAJOR == 1 && STXXL_VERSION_MINOR >= 4)
+    auto manager = stxxl::block_manager::get_instance();
+    util::Log() << "STXXL: peak bytes used: " << manager->get_maximum_allocation();
+    util::Log() << "STXXL: total disk allocated: " << manager->get_total_bytes();
+#else
+#warning STXXL 1.4+ recommended - STXXL memory summary will not be available
+    util::Log() << "STXXL: memory summary not available, needs STXXL 1.4 or higher";
+#endif
+
+#ifndef _WIN32
+    rusage usage;
+    getrusage(RUSAGE_SELF, &usage);
+#ifdef __linux__
+    // Under linux, ru.maxrss is in kb
+    util::Log() << "RAM: peak bytes used: " << usage.ru_maxrss * 1024;
+#else  // __linux__
+    // Under BSD systems (OSX), it's in bytes
+    util::Log() << "RAM: peak bytes used: " << usage.ru_maxrss;
+#endif // __linux__
+#else  // _WIN32
+    util::Log() << "RAM: peak bytes used: <not implemented on Windows>";
+#endif // _WIN32
+}
+}
+}
+
+#endif

scripts/install_node.sh

@@ -1,11 +1,18 @@
 #!/usr/bin/env bash
 
-# here we set up the node version on the fly. currently only node 4, but can be used for more values if need be
-# This is done manually so that the build works the same on OS X
-rm -rf ~/.nvm/ && git clone --depth 1 --branch v0.30.1 https://github.com/creationix/nvm.git ~/.nvm
-source ~/.nvm/nvm.sh
-nvm install $1
-nvm use $1
+NODE_HOME=$HOME/node
+export NODE_HOME
+mkdir ${NODE_HOME}
+if [ "${TRAVIS_OS_NAME}" == "osx" ]; then
+  curl https://s3.amazonaws.com/mapbox/apps/install-node/v2.0.0/run | NV=4.4.2 NP=darwin-x64 OD=${NODE_HOME} sh
+else
+  curl https://s3.amazonaws.com/mapbox/apps/install-node/v2.0.0/run | NV=4.4.2 NP=linux-x64 OD=${NODE_HOME} sh
+fi
+
+PATH="${NODE_HOME}/bin:$PATH"
+export PATH
 node --version
 npm --version
 which node
+
+

src/contractor/contractor.cpp

@@ -136,12 +136,24 @@ int Contractor::Run()
 
     TIMER_START(preparing);
 
+    util::Log() << "Reading node weights.";
+    std::vector<EdgeWeight> node_weights;
+    std::string node_file_name = config.osrm_input_path.string() + ".enw";
+
+    {
+        storage::io::FileReader node_file(node_file_name,
+                                          storage::io::FileReader::VerifyFingerprint);
+        node_file.DeserializeVector(node_weights);
+    }
+    util::Log() << "Done reading node weights.";
+
     util::Log() << "Loading edge-expanded graph representation";
 
     util::DeallocatingVector<extractor::EdgeBasedEdge> edge_based_edge_list;
 
     EdgeID max_edge_id = LoadEdgeExpandedGraph(config.edge_based_graph_path,
                                                edge_based_edge_list,
+                                               node_weights,
                                                config.edge_segment_lookup_path,
                                                config.edge_penalty_path,
                                                config.segment_speed_lookup_paths,
@@ -163,17 +175,6 @@ int Contractor::Run()
         ReadNodeLevels(node_levels);
     }
 
-    util::Log() << "Reading node weights.";
-    std::vector<EdgeWeight> node_weights;
-    std::string node_file_name = config.osrm_input_path.string() + ".enw";
-
-    {
-        storage::io::FileReader node_file(node_file_name,
-                                          storage::io::FileReader::VerifyFingerprint);
-        node_file.DeserializeVector(node_weights);
-    }
-    util::Log() << "Done reading node weights.";
-
     util::DeallocatingVector<QueryEdge> contracted_edge_list;
     ContractGraph(max_edge_id,
                   edge_based_edge_list,
@@ -499,6 +500,7 @@ parse_turn_penalty_lookup_from_csv_files(const std::vector<std::string> &turn_pe
 EdgeID Contractor::LoadEdgeExpandedGraph(
     std::string const &edge_based_graph_filename,
     util::DeallocatingVector<extractor::EdgeBasedEdge> &edge_based_edge_list,
+    std::vector<EdgeWeight> &node_weights,
     const std::string &edge_segment_lookup_filename,
     const std::string &edge_penalty_filename,
     const std::vector<std::string> &segment_speed_filenames,
@@ -914,6 +916,11 @@ EdgeID Contractor::LoadEdgeExpandedGraph(
                 previous_osm_node_id = segmentblocks[i].this_osm_node_id;
             }
 
+            // Update the node-weight cache.  This is the weight of the edge-based-node only,
+            // it doesn't include the turn.  We may visit the same node multiple times, but
+            // we should always assign the same value here.
+            node_weights[inbuffer.source] = new_weight;
+
             // We found a zero-speed edge, so we'll skip this whole edge-based-edge which
             // effectively removes it from the routing network.
             if (skip_this_edge)

src/extractor/guidance/coordinate_extractor.cpp

@@ -132,7 +132,7 @@ util::Coordinate CoordinateExtractor::ExtractRepresentativeCoordinate(
     {
         const auto result = ExtractCoordinateAtLength(
             skipping_inaccuracies_distance, coordinates);
-        BOOST_ASSERT(is_valid_result(coordinates.back()));
+        BOOST_ASSERT(is_valid_result(result));
         return result;
     }
 

src/extractor/guidance/sliproad_handler.cpp

@@ -186,11 +186,9 @@ operator()(const NodeID /*nid*/, const EdgeID source_edge_id, Intersection inter
             if (is_left_sliproad_turn)
                 return main_road_intersection->intersection.getLeftmostRoad();
 
-            if (is_right_sliproad_turn)
-                return main_road_intersection->intersection.getRightmostRoad();
-
-            BOOST_ASSERT_MSG(false, "Sliproad is neither a left nor right of obvious main road");
-            return main_road_intersection->intersection.getLeftmostRoad();
+            BOOST_ASSERT_MSG(is_right_sliproad_turn,
+                             "Sliproad is neither a left nor right of obvious main road");
+            return main_road_intersection->intersection.getRightmostRoad();
         }();
 
         const auto &crossing_road_data = node_based_graph.GetEdgeData(crossing_road.eid);

src/tools/contract.cpp

@@ -15,6 +15,8 @@
 #include <new>
 #include <ostream>
 
+#include "util/meminfo.hpp"
+
 using namespace osrm;
 
 enum class return_code : unsigned
@@ -166,7 +168,11 @@ int main(int argc, char *argv[]) try
 
     tbb::task_scheduler_init init(contractor_config.requested_num_threads);
 
-    return contractor::Contractor(contractor_config).Run();
+    auto exitcode = contractor::Contractor(contractor_config).Run();
+
+    util::DumpMemoryStats();
+
+    return exitcode;
 }
 catch (const std::bad_alloc &e)
 {

src/tools/extract.cpp

@@ -13,6 +13,8 @@
 #include <exception>
 #include <new>
 
+#include "util/meminfo.hpp"
+
 using namespace osrm;
 
 enum class return_code : unsigned
@@ -153,7 +155,11 @@ int main(int argc, char *argv[]) try
     // setup scripting environment
     extractor::LuaScriptingEnvironment scripting_environment(
         extractor_config.profile_path.string().c_str());
-    return extractor::Extractor(extractor_config).run(scripting_environment);
+    auto exitcode = extractor::Extractor(extractor_config).run(scripting_environment);
+
+    util::DumpMemoryStats();
+
+    return exitcode;
 }
 catch (const std::bad_alloc &e)
 {

unit_tests/CMakeLists.txt

@@ -57,7 +57,7 @@ target_include_directories(util-tests PUBLIC ${CMAKE_CURRENT_SOURCE_DIR})
 
 target_link_libraries(engine-tests ${ENGINE_LIBRARIES} ${Boost_UNIT_TEST_FRAMEWORK_LIBRARY})
 target_link_libraries(extractor-tests ${EXTRACTOR_LIBRARIES} ${Boost_UNIT_TEST_FRAMEWORK_LIBRARY})
-target_link_libraries(library-tests osrm ${Boost_UNIT_TEST_FRAMEWORK_LIBRARY})
+target_link_libraries(library-tests osrm ${ENGINE_LIBRARIES} ${Boost_UNIT_TEST_FRAMEWORK_LIBRARY})
 target_link_libraries(server-tests osrm ${Boost_UNIT_TEST_FRAMEWORK_LIBRARY})
 target_link_libraries(util-tests ${UTIL_LIBRARIES} ${Boost_UNIT_TEST_FRAMEWORK_LIBRARY})
 

unit_tests/library/json.cpp

@@ -0,0 +1,71 @@
+#include <boost/test/test_case_template.hpp>
+#include <boost/test/unit_test.hpp>
+
+#include "coordinates.hpp"
+#include "equal_json.hpp"
+
+#include "engine/api/json_factory.hpp"
+#include "osrm/coordinate.hpp"
+
+#include <iterator>
+#include <vector>
+
+using namespace osrm;
+
+BOOST_AUTO_TEST_SUITE(json)
+
+BOOST_AUTO_TEST_CASE(test_json_linestring)
+{
+    const std::vector<util::Coordinate> locations{get_dummy_location(),  //
+                                                  get_dummy_location(),  //
+                                                  get_dummy_location()}; //
+
+    auto geom = engine::api::json::makeGeoJSONGeometry(begin(locations), end(locations));
+
+    const auto type = geom.values["type"].get<util::json::String>().value;
+    BOOST_CHECK_EQUAL(type, "LineString");
+
+    const auto coords = geom.values["coordinates"].get<util::json::Array>().values;
+    BOOST_CHECK_EQUAL(coords.size(), 3); // array of three location arrays
+
+    for (const auto each : coords)
+    {
+        const auto loc = each.get<util::json::Array>().values;
+        BOOST_CHECK_EQUAL(loc.size(), 2);
+
+        const auto lon = loc[0].get<util::json::Number>().value;
+        const auto lat = loc[1].get<util::json::Number>().value;
+
+        (void)lon;
+        (void)lat;
+        // cast fails if type do not match
+    }
+}
+
+BOOST_AUTO_TEST_CASE(test_json_single_point)
+{
+    const std::vector<util::Coordinate> locations{get_dummy_location()};
+
+    auto geom = engine::api::json::makeGeoJSONGeometry(begin(locations), end(locations));
+
+    const auto type = geom.values["type"].get<util::json::String>().value;
+    BOOST_CHECK_EQUAL(type, "LineString");
+
+    const auto coords = geom.values["coordinates"].get<util::json::Array>().values;
+    BOOST_CHECK_EQUAL(coords.size(), 2); // array of two location arrays
+
+    for (const auto each : coords)
+    {
+        const auto loc = each.get<util::json::Array>().values;
+        BOOST_CHECK_EQUAL(loc.size(), 2);
+
+        const auto lon = loc[0].get<util::json::Number>().value;
+        const auto lat = loc[1].get<util::json::Number>().value;
+
+        (void)lon;
+        (void)lat;
+        // cast fails if type do not match
+    }
+}
+
+BOOST_AUTO_TEST_SUITE_END()