From d3ab6f1fcadbabe0a90a59c19aa561b836cf5e4a Mon Sep 17 00:00:00 2001 From: Michael Bell Date: Tue, 22 Sep 2020 21:32:35 +0100 Subject: [PATCH 1/3] Remove unused future The serialization of the compressed node based graph was changed in c410c2 to no longer be asynchronous. This removes the unused future object. --- src/extractor/extractor.cpp | 42 ++++++------------------------------- 1 file changed, 6 insertions(+), 36 deletions(-) diff --git a/src/extractor/extractor.cpp b/src/extractor/extractor.cpp index 6892232d3..3fb5ff1f0 100644 --- a/src/extractor/extractor.cpp +++ b/src/extractor/extractor.cpp @@ -1,5 +1,6 @@ #include "extractor/extractor.hpp" +#include "extractor/compressed_edge_container.hpp" #include "extractor/compressed_node_based_graph_edge.hpp" #include "extractor/edge_based_edge.hpp" #include "extractor/extraction_containers.hpp" @@ -11,47 +12,34 @@ #include "extractor/maneuver_override_relation_parser.hpp" #include "extractor/name_table.hpp" #include "extractor/node_based_graph_factory.hpp" -#include "extractor/raster_source.hpp" #include "extractor/restriction_filter.hpp" +#include "extractor/restriction_index.hpp" #include "extractor/restriction_parser.hpp" #include "extractor/scripting_environment.hpp" +#include "extractor/tarjan_scc.hpp" +#include "extractor/way_restriction_map.hpp" #include "guidance/files.hpp" #include "guidance/guidance_processing.hpp" #include "guidance/segregated_intersection_classification.hpp" #include "guidance/turn_data_container.hpp" -#include "storage/io.hpp" - #include "util/exception.hpp" #include "util/exception_utils.hpp" #include "util/integer_range.hpp" #include "util/log.hpp" -#include "util/range_table.hpp" -#include "util/timing_util.hpp" - -#include "extractor/compressed_edge_container.hpp" -#include "extractor/restriction_index.hpp" -#include "extractor/way_restriction_map.hpp" #include "util/static_graph.hpp" #include "util/static_rtree.hpp" +#include "util/timing_util.hpp" // Keep debug include to make sure the debug header is in sync with types. #include "util/debug.hpp" -#include "extractor/tarjan_scc.hpp" - #include -#include -#include -#include -#include -#include #include #include #include -#include #include #include @@ -62,15 +50,11 @@ #endif #include -#include - #include #include #include #include -#include #include -#include #include #include #include @@ -193,7 +177,7 @@ std::vector toEdgeList(const util::NodeBasedDynami return edges; } -} +} // namespace /** * TODO: Refactor this function into smaller functions for better readability. @@ -288,16 +272,6 @@ int Extractor::run(ScriptingEnvironment &scripting_environment) // // Luckily node based node ids still coincide with the coordinate array. // That's the reason we can only here write out the final compressed node based graph. - - // Dumps to file asynchronously and makes sure we wait for its completion. - std::future compressed_node_based_graph_writing; - - BOOST_SCOPE_EXIT_ALL(&) - { - if (compressed_node_based_graph_writing.valid()) - compressed_node_based_graph_writing.wait(); - }; - files::writeCompressedNodeBasedGraph(config.GetPath(".osrm.cnbg").string(), toEdgeList(node_based_graph)); @@ -519,7 +493,6 @@ Extractor::ParseOSMData(ScriptingEnvironment &scripting_environment, // OSM elements Lua parser tbb::filter_t buffer_transformer( tbb::filter::parallel, [&](const SharedBuffer buffer) { - ParsedBuffer parsed_buffer; parsed_buffer.buffer = buffer; scripting_environment.ProcessElements(*buffer, @@ -540,7 +513,6 @@ Extractor::ParseOSMData(ScriptingEnvironment &scripting_environment, unsigned number_of_maneuver_overrides = 0; tbb::filter_t buffer_storage( tbb::filter::serial_in_order, [&](const ParsedBuffer &parsed_buffer) { - number_of_nodes += parsed_buffer.resulting_nodes.size(); // put parsed objects thru extractor callbacks for (const auto &result : parsed_buffer.resulting_nodes) @@ -564,7 +536,6 @@ Extractor::ParseOSMData(ScriptingEnvironment &scripting_environment, { extractor_callbacks->ProcessManeuverOverride(result); } - }); tbb::filter_t> buffer_relation_cache( @@ -606,7 +577,6 @@ Extractor::ParseOSMData(ScriptingEnvironment &scripting_environment, tbb::filter_t, void> buffer_storage_relation( tbb::filter::serial_in_order, [&](const std::shared_ptr parsed_relations) { - number_of_relations += parsed_relations->GetRelationsNum(); relations.Merge(std::move(*parsed_relations)); }); From 4799b46eeb934278d9d101f682a95da2b66af621 Mon Sep 17 00:00:00 2001 From: Michael Bell Date: Thu, 1 Oct 2020 02:44:22 +0100 Subject: [PATCH 2/3] Incorrect error message when unable to snap all input coordinates (#5846) In cases where we are unable to find a phantom node for an input coordinate, we return an error indicating which coordinate failed. This would always refer to the coordinate with index equal to the number of valid phantom nodes found. We fix this by instead returning the first index for which a phantom node could not be found. --- CHANGELOG.md | 1 + include/engine/plugins/plugin_base.hpp | 16 ++++++++++++++++ src/engine/plugins/table.cpp | 6 ++---- src/engine/plugins/trip.cpp | 3 +-- src/engine/plugins/viaroute.cpp | 3 +-- unit_tests/library/table.cpp | 2 ++ 6 files changed, 23 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c44f9ab8e..c2f756194 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ - CHANGED: Reduce memory usage for raster source handling. [#5572](https://github.com/Project-OSRM/osrm-backend/pull/5572) - CHANGED: Add cmake option `ENABLE_DEBUG_LOGGING` to control whether output debug logging. [#3427](https://github.com/Project-OSRM/osrm-backend/issues/3427) - CHANGED: updated extent of Hong Kong as left hand drive country. [#5535](https://github.com/Project-OSRM/osrm-backend/issues/5535) + - FIXED: corrected error message when failing to snap input coordinates [#5846](https://github.com/Project-OSRM/osrm-backend/pull/5846) - Infrastructure - REMOVED: STXXL support removed as STXXL became abandonware. [#5760](https://github.com/Project-OSRM/osrm-backend/pull/5760) # 5.21.0 diff --git a/include/engine/plugins/plugin_base.hpp b/include/engine/plugins/plugin_base.hpp index 28e3db72f..6fc23adcd 100644 --- a/include/engine/plugins/plugin_base.hpp +++ b/include/engine/plugins/plugin_base.hpp @@ -371,6 +371,22 @@ class BasePlugin } return phantom_node_pairs; } + + std::string MissingPhantomErrorMessage(const std::vector &phantom_nodes, + const std::vector &coordinates) const + { + BOOST_ASSERT(phantom_nodes.size() < coordinates.size()); + auto mismatch = std::mismatch(phantom_nodes.begin(), + phantom_nodes.end(), + coordinates.begin(), + coordinates.end(), + [](const auto &phantom_node, const auto &coordinate) { + return phantom_node.first.input_location == coordinate; + }); + std::size_t missing_index = std::distance(phantom_nodes.begin(), mismatch.first); + return std::string("Could not find a matching segment for coordinate ") + + std::to_string(missing_index); + } }; } } diff --git a/src/engine/plugins/table.cpp b/src/engine/plugins/table.cpp index 190a0138c..5b517ac17 100644 --- a/src/engine/plugins/table.cpp +++ b/src/engine/plugins/table.cpp @@ -75,10 +75,8 @@ Status TablePlugin::HandleRequest(const RoutingAlgorithmsInterface &algorithms, if (phantom_nodes.size() != params.coordinates.size()) { - return Error("NoSegment", - std::string("Could not find a matching segment for coordinate ") + - std::to_string(phantom_nodes.size()), - result); + return Error( + "NoSegment", MissingPhantomErrorMessage(phantom_nodes, params.coordinates), result); } auto snapped_phantoms = SnapPhantomNodes(phantom_nodes); diff --git a/src/engine/plugins/trip.cpp b/src/engine/plugins/trip.cpp index 75a38d50a..74a4452aa 100644 --- a/src/engine/plugins/trip.cpp +++ b/src/engine/plugins/trip.cpp @@ -199,8 +199,7 @@ Status TripPlugin::HandleRequest(const RoutingAlgorithmsInterface &algorithms, if (phantom_node_pairs.size() != number_of_locations) { return Error("NoSegment", - std::string("Could not find a matching segment for coordinate ") + - std::to_string(phantom_node_pairs.size()), + MissingPhantomErrorMessage(phantom_node_pairs, parameters.coordinates), result); } BOOST_ASSERT(phantom_node_pairs.size() == number_of_locations); diff --git a/src/engine/plugins/viaroute.cpp b/src/engine/plugins/viaroute.cpp index 8c4e3e891..9b0f3ebfc 100644 --- a/src/engine/plugins/viaroute.cpp +++ b/src/engine/plugins/viaroute.cpp @@ -90,8 +90,7 @@ Status ViaRoutePlugin::HandleRequest(const RoutingAlgorithmsInterface &algorithm if (phantom_node_pairs.size() != route_parameters.coordinates.size()) { return Error("NoSegment", - std::string("Could not find a matching segment for coordinate ") + - std::to_string(phantom_node_pairs.size()), + MissingPhantomErrorMessage(phantom_node_pairs, route_parameters.coordinates), result); } BOOST_ASSERT(phantom_node_pairs.size() == route_parameters.coordinates.size()); diff --git a/unit_tests/library/table.cpp b/unit_tests/library/table.cpp index 139830856..26fbdec78 100644 --- a/unit_tests/library/table.cpp +++ b/unit_tests/library/table.cpp @@ -242,6 +242,8 @@ BOOST_AUTO_TEST_CASE(test_table_no_segment_for_some_coordinates) BOOST_CHECK(rc == Status::Error); const auto code = json_result.values.at("code").get().value; BOOST_CHECK_EQUAL(code, "NoSegment"); + const auto message = json_result.values.at("message").get().value; + BOOST_CHECK_EQUAL(message, "Could not find a matching segment for coordinate 0"); } BOOST_AUTO_TEST_CASE(test_table_serialiaze_fb) From 3451d1ec8237914b4a598e6f5f82147d5a45bf77 Mon Sep 17 00:00:00 2001 From: Daniel Patterson Date: Wed, 30 Sep 2020 18:45:44 -0700 Subject: [PATCH 3/3] Lock access to facade_factory in data_watchdog to avoid accessing destructed object (#5844) * Wrap access to facade_factory in a shared lock so it doesn't get changed partway through access which leads to a crash. --- CHANGELOG.md | 1 + include/engine/data_watchdog.hpp | 31 +++++++++++++++++++++---------- 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c2f756194..a7eab1b7a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ - CHANGED: default car weight was reduced to 2000 kg. [#5371](https://github.com/Project-OSRM/osrm-backend/pull/5371) - CHANGED: default car height was reduced to 2 meters. [#5389](https://github.com/Project-OSRM/osrm-backend/pull/5389) - FIXED: treat `bicycle=use_sidepath` as no access on the tagged way. [#5622](https://github.com/Project-OSRM/osrm-backend/pull/5622) + - FIXED: fix occasional segfault when swapping data with osrm-datastore and using `exclude=` [#5844](https://github.com/Project-OSRM/osrm-backend/pull/5844) - Misc: - CHANGED: Reduce memory usage for raster source handling. [#5572](https://github.com/Project-OSRM/osrm-backend/pull/5572) - CHANGED: Add cmake option `ENABLE_DEBUG_LOGGING` to control whether output debug logging. [#3427](https://github.com/Project-OSRM/osrm-backend/issues/3427) diff --git a/include/engine/data_watchdog.hpp b/include/engine/data_watchdog.hpp index b44274545..416f1df76 100644 --- a/include/engine/data_watchdog.hpp +++ b/include/engine/data_watchdog.hpp @@ -56,11 +56,14 @@ class DataWatchdogImpl( - std::make_shared( - std::vector{ - static_region.shm_key, updatable_region.shm_key})); + { + boost::unique_lock swap_lock(factory_mutex); + facade_factory = + DataFacadeFactory( + std::make_shared( + std::vector{ + static_region.shm_key, updatable_region.shm_key})); + } } watcher = std::thread(&DataWatchdogImpl::Run, this); @@ -75,10 +78,14 @@ class DataWatchdogImpl Get(const api::BaseParameters ¶ms) const { + // make sure facade_factory stays stable while we call Get() + boost::shared_lock swap_lock(factory_mutex); return facade_factory.Get(params); } std::shared_ptr Get(const api::TileParameters ¶ms) const { + // make sure facade_factory stays stable while we call Get() + boost::shared_lock swap_lock(factory_mutex); return facade_factory.Get(params); } @@ -111,16 +118,20 @@ class DataWatchdogImpl( - std::make_shared( - std::vector{ - static_region.shm_key, updatable_region.shm_key})); + { + boost::unique_lock swap_lock(factory_mutex); + facade_factory = + DataFacadeFactory( + std::make_shared( + std::vector{ + static_region.shm_key, updatable_region.shm_key})); + } } util::Log() << "DataWatchdog thread stopped"; } + mutable boost::shared_mutex factory_mutex; const std::string dataset_name; storage::SharedMonitor barrier; std::thread watcher;