Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue with cache #4985

Merged
merged 2 commits into from
May 3, 2023
Merged

Conversation

Helium314
Copy link
Collaborator

see #4980 (comment)

No tests yet, so far I failed to reproduce the bug in a unit test.

@westnordost
Copy link
Member

TBH I understand neither the problem nor the solution. But this is not a requirement, unless you need feedback from me.

@Helium314
Copy link
Collaborator Author

Helium314 commented May 3, 2023

What happens:
Tile 1 is cached from a previous fetch, then getMapDataWithGeometry is called for a bbox (exactly) containing tile 1 and adjacent tile 2.
tilesToFetch will contain tile 2 only, because tile 1 is already cached.
In nodes = HashSet<Node>(spatialCache.get(bbox)), the SpatialCache will use its own fetch, but SpatialCache.fetch always returns an empty list for tile 2. This adds tile 2 to the SpatialCache, containing the empty list returned by SpatialCache.fetch.
At this point there is no issue, because the tile being empty is expected, and we replace the fetched empty tile 2 with the actual data as part of the immediately following update.
But during the update, the tilesRect.asBoundingBox(zoom).enclosingTilesRect(zoom) bug(?) sometimes kicks in, and the unexpectedly extended larger tiles rect suddenly contains tile 1 and tile 2, instead of tile 2 only. Tile 1 is then seen as incomplete, because it's not fully in the bbox that was supposed to contain tile 2 only.
Since tile 1 is supposed to be replaced with incomplete data, tile 1 is removed from SpatialCache.

Next, MapDataCache fetches the nodes added to SpatialCache in update, and for shorter code it was done using SpatialCache.get. I had seen this as safe, but now we know it's not.
As described above, when now calling get for the full bbox of tile 1 and tile 2, the previously removed tile 1 is fetched using SpatialCache.fetch, so it contains no nodes. But this time MapDataCache isn't aware of this, and the empty tile is seen as properly cached. On this fetch, the returned data is still correct (edit: or maybe not? doesn't really matter anyway). On further fetches in tile 1, nothing will be returned.

With this PR, the second SpatialCache.fetch is not done, and thus if the tilesRect.asBoundingBox(zoom).enclosingTilesRect(zoom) bug occurs, the removed tile 1 is simply fetched again from DB.

@Helium314
Copy link
Collaborator Author

added a test that fails without the change to MapDataCache

@Helium314
Copy link
Collaborator Author

I'll merge this, as it works, and with the PR merged I'll continue #4980 (might actually be finished already, assuming the issues I observed were really caused by this bug only).

@Helium314 Helium314 merged commit fed28e4 into streetcomplete:master May 3, 2023
@Helium314 Helium314 deleted the Helium314-patch-2 branch May 3, 2023 06:29
@westnordost
Copy link
Member

Thank you for the explanation.

It makes sense to not rely on tilesRect.asBoundingBox(zoom).enclosingTilesRect(zoom) == tilesRect because when one data structure is inclusive and the other exclusive to its bound, it is somewhat difficult or impossible to have a lossless conversion between the two.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants