Slip lane detection: track visited nodes to avoid infinite loops

[merkispavel]

Issue

There was an edge with 2 extremely close nodes in the data and distance threshold was never exhausted because edge length was zero. Finally it caused infinite loop and OOM https://github.com/valhalla/valhalla/blob/master/src/mjolnir/linkclassification.cc#L561

The issue is gone now after @gknisely's OSM fix . But it makes sense prevent such looping in the code

This PR adds more robust way not to loop(track all nodes instead of tracking previous edge)

Tests:

• the pbf around the buggy location is built properly after the fix (loop.osm.pbf.zip)
• no regressions in turn channel count on maryland.osm.pbf

Tasklist

• Add tests
• Add #fixes with the issue number that this PR addresses
• Update the docs with any new request parameters or changes to behavior described
• Update the changelog
• If you made changes to the lua files, update the taginfo too.

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

[merkispavel]

changed the order of check to put hash set lookup in the end

[genadz]

make sense

[mandeepsandhu]

Is it possible to add a test with 0-length edges in a gurka map?

[merkispavel]

Is it possible to add a test with 0-length edges in a gurka map?

@mandeepsandhu reasonable question. I thought about it. My view is it should be possible in theory but it can take much efforts as this bug depends on the order of edges in the data so the test can become useless after code tweaks in the future.
I guess I can give it a try(spend an hour or around and try to reproduce it). I'll let you know if I succeed or not

[mandeepsandhu]

Is it possible to add a test with 0-length edges in a gurka map?

@mandeepsandhu reasonable question. I thought about it. My view is it should be possible in theory but it can take much efforts as this bug depends on the order of edges in the data so the test can become useless after code tweaks in the future.
I guess I can give it a try(spend an hour or around and try to reproduce it). I'll let you know if I succeed or not

Yeah, you dont have to spend too much time trying to recreate it in gurka (especially if the tests is going to be fragile).

BTW, is it possible to test with a OSM extract of the area where @gknisely made the edit? I dont know if cutting a small pbf around that area can help recreate the issue.

[merkispavel]

@mandeepsandhu I believe valhalla policy is not to use osm extracts and replace them with gurka tests

I dont know if cutting a small pbf around that area can help recreate the issue.

That's exactly how I tested the PR but locally

[mandeepsandhu]

@mandeepsandhu I believe valhalla policy is not to use osm extracts and replace them with gurka tests

True, in that we prefer to add gurka tests for most of our tests. Although I believe exceptions can be made if its difficult or hard-to-reproduce the bug in gurka. This code might change in the future so it'll be good to have some form of test to catch possible regressions.

[merkispavel]

Is it possible to add a test with 0-length edges in a gurka map?

@mandeepsandhu reasonable question. I thought about it. My view is it should be possible in theory but it can take much efforts as this bug depends on the order of edges in the data so the test can become useless after code tweaks in the future.
I guess I can give it a try(spend an hour or around and try to reproduce it). I'll let you know if I succeed or not

Yeah, you dont have to spend too much time trying to recreate it in gurka (especially if the tests is going to be fragile).

BTW, is it possible to test with a OSM extract of the area where @gknisely made the edit? I dont know if cutting a small pbf around that area can help recreate the issue.

@mandeepsandhu There's some dark magic happened with this extract. @gknisely and I, we tried to reproduce it with a gurka test but without success. Going to merge it as it is. Local testing should be enough to be safe to merge it(though would be better with gurka 😢 )

note: here's an extract how to assign the same coordinates for two nodes, maybe it'll be helpful in the future

  itr = layout.find("D");
  EXPECT_NE(itr, layout.end());
  midgard::PointLL ll = itr->second;

  itr = layout.find("C");
  EXPECT_NE(itr, layout.end());
  itr->second = ll;