Fix a "ghost points" scenario #27

hackergrrl · 2017-01-10T02:18:39Z

This happens where a map query is made with 'forks' disabled, meaning only
the latest version of each OSM element is returned.

In some cases an OSM 'way' that is excluded will reference OSM nodes that
are not present in the way that is ultimately returned. Prior to this
commit, those ways' unreferenced nodes would be returned regardless,
resulting in the final map being peppered with unlabeled
"ghost points", with no way referencing them.

This fixes the issue by tracking which nodes are referenced to by ways that
are excluded, and filtering them out of the final result.

This happens where a map query is made with 'forks' disabled, meaning only the latest version of each OSM element is returned. In some cases an OSM 'way' that is excluded will reference OSM nodes that are not present in the way that is ultimately returned. Prior to this commit, those ways' unreferenced nodes would be returned regardless, resulting in the final map being peppered with unlabeled "ghost points", with no way referencing them. This fixes the issue by tracking which nodes are referenced to by ways that are excluded, and filtering them out of the final result.

gmaclennan · 2017-01-10T13:18:54Z

routes/misc_map.js

@@ -29,13 +29,7 @@ module.exports = function (req, res, api, params, next) {
  }
  api.getMap(bbox, function (err, elements) {


This fork logic should be in https://github.com/digidem/osm-p2p-server/blob/master/api/get_map.js I think. I see now an unrelated error: the switch (accept.types(['xml', 'json'])) should be called whether query.forks is true or false - I introduced this bug when I introduced the json endpoint.

Done and fixed.

gmaclennan

Can we move the logic to api/get_map.js? I think it makes more sense there. We could extract the filterForkElements function and test that independently, which would make the test simpler - we just test that function.

gmaclennan

Sorry somehow started another "review" instead of just leaving a comment

gmaclennan · 2017-01-10T13:28:47Z

routes/misc_map.js

+    // Remove excluded entries that appear in the keep entries.
+    Object.keys(keepNodeRefs).forEach(function (ref) {
+      if (excludeNodeRefs[ref]) {
+        delete excludeNodeRefs[ref]


This will be slow and can cause memory issues. I think that looking at line #94 you don't actually need this, but my head is a little slow right now...

I see an opportunity to expand my Node knowledge! Why will this be slow? What memory issues will it cause?

Removed; though I'm still curious to understand the performance concerns.

The route logic for /bbox does will not return json even if it is requested.

hackergrrl · 2017-01-11T00:32:32Z

Moved the filter piece to the API layer.
Moved the filter logic into its own module in lib/.
Added unit tests for (2).
Simplified filtering logic to hopefully be more performant.

gmaclennan · 2017-01-11T00:53:01Z

I don't know that much, other than having a PR comment from MapBox for including delete. The issue I think is with using it in a loop, so performance impacts can add up. There is this: http://stackoverflow.com/questions/27397034/why-is-delete-slow-in-javascript And this with good discussion: googleapis/google-api-nodejs-client#375 I think because this object could be very large for a large forked way, and could be looped hundreds of times, this is a time where it is worth considering this kind of performance On Jan 11, 2017, at 12:35 AM, Stephen Whitmore <[email protected]> wrote: *@noffle* commented on this pull request. ------------------------------ In routes/misc_map.js <#27>:

+

+ // Note that all of the nodes referenced by this way should be culled. + if (element.type === 'way') { + element.nodes.forEach(function (ref) { + excludeNodeRefs[ref] = true + }) + } + + return false + } + }) + + // Remove excluded entries that appear in the keep entries. + Object.keys(keepNodeRefs).forEach(function (ref) { + if (excludeNodeRefs[ref]) { + delete excludeNodeRefs[ref] Removed; though I'm still curious to understand the performance concerns. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#27>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARumRcRCLyXXTHMH2iqafh4_sGAC9npks5rRCO6gaJpZM4Le9US> .

hackergrrl · 2017-01-11T02:38:54Z

I added some more tests to cover the scenario where there are two forks to a way: one modifies a point, one deletes a point.

gmaclennan · 2017-01-12T15:29:14Z

Sorry to report bad news... Unfortunately there seems to be some kind of race condition here, and test/ghost_points.js fails about 1 in 3 times:

not ok 19 should be equal
  ---
    operator: equal
    expected: 2
    actual:   3
    at: ConcatStream.<anonymous> (/Users/gregor/Dev/DdDev/osm-p2p-server/node_modules/concat-stream/index.js:36:43)
  ...
not ok 20 should be equivalent
  ---
    operator: deepEqual
    expected: |-
      [ '2864043637183826736', '9003328637408641236' ]
    actual: |-
      [ '2864043637183826736', '5274879986653545291', '9003328637408641236' ]
    at: ConcatStream.<anonymous> (/Users/gregor/Dev/DdDev/osm-p2p-server/node_modules/concat-stream/index.js:36:43)
  ...
ok 21 undefined

Unfortunately setting env OSM_P2P_DB_DELAY=200 does not seem to reliably reproduce this bug, which it is supposed to.

gmaclennan · 2017-01-12T15:53:44Z

I added slowdb() to the tests but still not reliably reproducing. This race condition may be coming from somewhere else, maybe in the test setup itself.

gmaclennan · 2017-01-12T16:31:16Z

Still getting random errors...

Without fully parsing your code, I suspect that this may be due to unstable sorting of forks because you are creating points directly (i.e. not with the osm-server api) without timestamps, so the sorting will be based on version numbers rather than timestamps.

gmaclennan · 2017-01-12T16:34:48Z

Ok, adding timestamps fixes this. I'm going to merge and cut a release so we can work with this today, but @noffle can you review this test once more to check I'm not missing anything with this fix?

hackergrrl added 7 commits January 9, 2017 17:36

Remove stdout debug statement.

e84c757

Create failing ghost point test.

e0e7968

Clean up osm-p2p-server test server on test completion.

b4e652f

Use run-waterfall to improve readability.

cc16508

Clarify test title.

90d4436

Make forked node culling logic a bit more readable.

efc28cd

hackergrrl changed the title ~~Fix "ghost points" bug.~~ Fix a "ghost points" bug scenario Jan 10, 2017

Clean up and error handling.

7be7c34

hackergrrl changed the title ~~Fix a "ghost points" bug scenario~~ Fix a "ghost points" scenario Jan 10, 2017

Do not exclude stand-alone nodes.

6b45447

gmaclennan reviewed Jan 10, 2017

View reviewed changes

gmaclennan requested changes Jan 10, 2017

View reviewed changes

hackergrrl added 7 commits January 10, 2017 14:48

Add failing bbox route test.

eec2c9d

The route logic for /bbox does will not return json even if it is requested.

Return json or xml on the /bbox route.

fe85d0f

Remove extraneous newline.

6e7278d

Move fork-aware filter logic to api/get_map.

cb90a26

Move collect-transform-stream into its own module.

1d4ce65

Move filterForkedElements to its own module.

d2f42a3

Add unit test for filterForkedElements.

b7e932e

hackergrrl added 2 commits January 10, 2017 16:35

Remove extraneous filtering logic.

bf1ed0c

Fix linting niggles.

003e300

hackergrrl added 3 commits January 10, 2017 18:27

Add unit test for way node deletion fork scenario.

fa1c370

Add integration test for delete/modify way fork.

f592e53

Linting.

7b56191

Remove superfulous setHeaders

0ec137e

Use slowdb with random delay with ghost_point test

3b9aed3

Ensure replication is complete and indexes ready in ghost point test

dcab96a

Add timestamps to nodes created in test

626f803

gmaclennan approved these changes Jan 12, 2017

View reviewed changes

gmaclennan merged commit 0494f02 into master Jan 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a "ghost points" scenario #27

Fix a "ghost points" scenario #27

hackergrrl commented Jan 10, 2017

gmaclennan Jan 10, 2017

hackergrrl Jan 11, 2017

gmaclennan left a comment

gmaclennan left a comment

gmaclennan Jan 10, 2017

hackergrrl Jan 10, 2017

hackergrrl Jan 11, 2017

hackergrrl commented Jan 11, 2017

gmaclennan commented Jan 11, 2017 via email

hackergrrl commented Jan 11, 2017

gmaclennan commented Jan 12, 2017

gmaclennan commented Jan 12, 2017

gmaclennan commented Jan 12, 2017

gmaclennan commented Jan 12, 2017

		@@ -29,13 +29,7 @@ module.exports = function (req, res, api, params, next) {
		}
		api.getMap(bbox, function (err, elements) {

Fix a "ghost points" scenario #27

Fix a "ghost points" scenario #27

Conversation

hackergrrl commented Jan 10, 2017

gmaclennan Jan 10, 2017

Choose a reason for hiding this comment

hackergrrl Jan 11, 2017

Choose a reason for hiding this comment

gmaclennan left a comment

Choose a reason for hiding this comment

gmaclennan left a comment

Choose a reason for hiding this comment

gmaclennan Jan 10, 2017

Choose a reason for hiding this comment

hackergrrl Jan 10, 2017

Choose a reason for hiding this comment

hackergrrl Jan 11, 2017

Choose a reason for hiding this comment

hackergrrl commented Jan 11, 2017

gmaclennan commented Jan 11, 2017 via email

hackergrrl commented Jan 11, 2017

gmaclennan commented Jan 12, 2017

gmaclennan commented Jan 12, 2017

gmaclennan commented Jan 12, 2017

gmaclennan commented Jan 12, 2017