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

Terrain availability race condition #11296

Merged
merged 6 commits into from
May 19, 2023
Merged

Terrain availability race condition #11296

merged 6 commits into from
May 19, 2023

Conversation

ggetz
Copy link
Contributor

@ggetz ggetz commented May 18, 2023

requestTileGeometry returns an error when layerToUse for that particular tile is undefined. Doing so marks the entire tile as failed, which means it does not ever get re-requested.

This is fine when we know all the tile availability up-front. In that case, the error is valid.

However, when cut-out terrain is used, it's possible to not know the availability for different layers at the time of request. The existing logic skipped over any layer with unknown availability for that tile, and if there were no other valid layers, then this tile was marked as failed.

This PR adjust things so that we don't continue with the request unless:

  1. We find a layer where the tile that is definitely available
  2. All of the availability data is ready (in some cases, this could still technically result in the error)

We can't do an early return in requestTileGeometry when availability is unknown or the tiles get re-requested too quickly and block other valid requests, ultimately leading to stalling. checkLayer caches results to prevent repeat requests, so its not problematic to call in quick succession.

This problem only really becomes apparent when there are a lot of layers with competing requests to get availability data.

@mramato Can you take a look?

@ggetz ggetz requested a review from mramato May 18, 2023 21:04
@cesium-concierge
Copy link

Thanks for the pull request @ggetz!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.

@ggetz ggetz marked this pull request as ready for review May 18, 2023 21:05
@mramato
Copy link
Contributor

mramato commented May 19, 2023

@ggetz This makes sense and seems like a good fix to me. Terrain that used to trigger an error now seems to be error free.

Would it be possible to add a unit test for this?

@ggetz
Copy link
Contributor Author

ggetz commented May 19, 2023

@mramato Yes, unit test added.

@mramato
Copy link
Contributor

mramato commented May 19, 2023

Thanks, @ggetz! Confirmed the new unit test fails in main and passes in this branch.

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.

3 participants