diff --git a/CHANGELOG.md b/CHANGELOG.md index 36c391c486..12580889a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,7 @@ - CHANGED: Upgrade Cucumber-js to v12 [#7221](https://github.com/Project-OSRM/osrm-backend/pull/7221) - CHANGED: Add libcap-setcap to alpine dockerfile [#7241](https://github.com/Project-OSRM/osrm-backend/issues/7241) - FIXED: Minor misspellings found in source code, comments and documents [#7215](https://github.com/Project-OSRM/osrm-backend/pull/7215) + - ADDED: Regression tests for zero-speed segment updates and unit test for turn_id uniqueness [#7332](https://github.com/Project-OSRM/osrm-backend/issues/7332) - Profiles: - FIXED: Bicycle and foot profiles now don't route on motor roads [#6697](https://github.com/Project-OSRM/osrm-backend/pull/6697) - FIXED: Use `cycleway:both` if available. [#6179](https://github.com/Project-OSRM/osrm-backend/issues/6179) diff --git a/features/testbot/zero-speed-updates.feature b/features/testbot/zero-speed-updates.feature index ae5a99c7f7..1e8e3ca43a 100644 --- a/features/testbot/zero-speed-updates.feature +++ b/features/testbot/zero-speed-updates.feature @@ -189,3 +189,74 @@ Feature: Check zero speed updates | waypoints | trips | code | | a,b,c,d | | NoTrips | | d,b,c,a | | NoTrips | + + + Scenario: Closing intersection should not block turns at earlier intersections + Given the node map + """ + x + | + a - 1 - b - 2 - c - 3 - d + | + y + """ + + And the ways + | nodes | + | abcd | + | xb | + | cy | + And the contract extra arguments "--segment-speed-file {speeds_file}" + And the customize extra arguments "--segment-speed-file {speeds_file}" + # Node IDs (top-to-bottom, left-to-right): x=1, a=2, b=3, c=4, d=5, y=6 + # Close ALL segments connecting to node c (4): + # - b-c (3,4) and c-b (4,3) + # - c-d (4,5) and d-c (5,4) + # - c-y (4,6) and y-c (6,4) + And the speed file + """ + 3,4,0 + 4,3,0 + 4,5,0 + 5,4,0 + 4,6,0 + 6,4,0 + """ + + # Route from position 1 (on open segment a-b) to x should work + # because turn at b is not affected by closure at c + When I route I should get + | from | to | code | + | 1 | x | Ok | + | a | x | Ok | + + + Scenario: Closing segment should not cascade to parallel routes + Given the node map + """ + a - 1 - b - 2 - c + | | + d - 3 - e - 4 - f + """ + + And the ways + | nodes | + | abc | + | def | + | ad | + | cf | + And the contract extra arguments "--segment-speed-file {speeds_file}" + And the customize extra arguments "--segment-speed-file {speeds_file}" + # Node IDs (top-to-bottom, left-to-right): a=1, b=2, c=3, d=4, e=5, f=6 + # Close segment b-c (2,3) + And the speed file + """ + 2,3,0 + 3,2,0 + """ + + # Route on def should work, route a->f uses alternative via def + When I route I should get + | from | to | code | + | 3 | 4 | Ok | + | a | f | Ok | diff --git a/src/updater/updater.cpp b/src/updater/updater.cpp index 3a08db577e..b950ee5f56 100644 --- a/src/updater/updater.cpp +++ b/src/updater/updater.cpp @@ -779,8 +779,9 @@ Updater::LoadAndUpdateEdgeExpandedGraph(std::vector &e if (turn_weight_penalty < TurnPenalty{0}) { util::Log(logWARNING) - << "turn penalty " << turn_weight_penalty - << " is too negative: clamping turn weight to " << weight_min_value; + << "turn penalty " << turn_weight_penalty << " for turn_id " + << edge.data.turn_id << " (edge " << edge.source << " -> " << edge.target + << ") is too negative: clamping turn weight to " << weight_min_value; turn_weight_penalty = alias_cast(weight_min_value - new_weight); turn_weight_penalties[edge.data.turn_id] = turn_weight_penalty; } diff --git a/unit_tests/updater/turn_id_uniqueness.cpp b/unit_tests/updater/turn_id_uniqueness.cpp new file mode 100644 index 0000000000..bdda406f13 --- /dev/null +++ b/unit_tests/updater/turn_id_uniqueness.cpp @@ -0,0 +1,75 @@ +#include "extractor/edge_based_edge.hpp" + +#include + +#include +#include +#include + +BOOST_AUTO_TEST_SUITE(turn_id_uniqueness) + +using namespace osrm; + +// This test documents and verifies the invariant that turn_id is unique per edge. +// +// During extraction, EdgeBasedGraphFactory renumbers all edges so that each edge's +// turn_id equals its index in the edge list (turn_id = 0, 1, 2, ..., N-1). +// +// The parallel update loop in Updater::LoadAndUpdateEdgeExpandedGraph relies on this +// property: since each edge has a unique turn_id, parallel threads write to different +// slots in turn_weight_penalties[] without racing. + +BOOST_AUTO_TEST_CASE(turn_id_assignment_simulates_extraction) +{ + // Simulate the turn_id renumbering done by EdgeBasedGraphFactory: + // each edge's turn_id is set to its index in the edge list + std::vector edges(1000); + + // Assign turn_id = index (mimicking the extractor renumbering) + for (std::size_t i = 0; i < edges.size(); ++i) + { + edges[i].data.turn_id = static_cast(i); + } + + // Verify all turn_ids are unique + std::set turn_ids; + for (const auto &edge : edges) + { + auto [it, inserted] = turn_ids.insert(edge.data.turn_id); + BOOST_CHECK_MESSAGE(inserted, + "turn_id " << edge.data.turn_id + << " is not unique - parallel updates would race"); + } + + BOOST_CHECK_EQUAL(turn_ids.size(), edges.size()); +} + +BOOST_AUTO_TEST_CASE(parallel_update_safety_with_unique_turn_ids) +{ + // This test verifies that with unique turn_ids, parallel writes to different + // array indices are safe (no synchronization needed). + const std::size_t num_edges = 10000; + std::vector penalties(num_edges, 0); + std::vector edges(num_edges); + + // Assign unique turn_ids + for (std::size_t i = 0; i < edges.size(); ++i) + { + edges[i].data.turn_id = static_cast(i); + } + + // Simulate parallel updates (sequentially here, but the point is each write + // goes to a different index) + for (const auto &edge : edges) + { + penalties[edge.data.turn_id] = edge.data.turn_id + 100; + } + + // Verify each slot was written correctly + for (std::size_t i = 0; i < penalties.size(); ++i) + { + BOOST_CHECK_EQUAL(penalties[i], static_cast(i + 100)); + } +} + +BOOST_AUTO_TEST_SUITE_END()