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 crash from accessing deleted SubtreeFileReader #775

Merged

Conversation

pjanetzek
Copy link

This change keeps SubtreeFileReader instance in the loaders future context. Should fix #774

keep SubtreeFileReader instance in the loaders future context
@j9liu
Copy link
Contributor

j9liu commented Dec 5, 2023

Thanks @pjanetzek! Before we review your code, can you sign a CLA (contributor license agreement) here?

@pjanetzek
Copy link
Author

Hi @j9liu as far as I know Peer signed the CLA for our team - could you please check if my account is on your list?

@kring
Copy link
Member

kring commented Dec 7, 2023

Yes we have a CLA for you, thanks @pjanetzek!

@kring
Copy link
Member

kring commented Dec 7, 2023

Thanks @pjanetzek! I pushed a few small changes:

  • Renamed a variable to match our conventions.
  • Modified a test to make it catch this bug. We have decent tests for availability subtree loading, but unfortunately the process is much more synchronous in the tests, so it didn't trigger this crash.
  • Added some documentation of this requirement that SubtreeFileReader remain valid until the returned future completes. I considered ways to eliminate this requirement, because it's not very intuitive, but the cost of every solution I could think of outweighs the benefit.
  • Updated CHANGES.md.

@kring kring merged commit 960d26e into CesiumGS:main Dec 7, 2023
7 checks passed
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.

Crash loading implicit tileset
4 participants