Fix another crash caused by destroying CesiumIonSession while it is doing network requests #499
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Merging this for tomorrow's release (it's very low risk). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In #479, I made some improvements to
CesiumIonSessionto a) keep the managed object alive while requests are in progress, and b) check that they haven't been destroyed (such as by an AppDomain unload) when the request completes. This significantly improved the stability of the tests, but I was still seeing an occasional access violation when running tests. For example, here:https://github.com/CesiumGS/cesium-unity/actions/runs/10658298813/job/29539138580
I was able to reproduce this on my machine by running the tests repeatedly, and the cause was that the CesiumIonSession was being destroyed while one of the
refreshmethods was running. Looks like I missed those when making the previous changes.This fix is an improvement, but still isn't perfect. During AppDomain unload, it's possible for the finalizer to run in another thread in between when the validaty of the session is checked and when we use the corresponding native object, which will still result in a crash. This PR narrows the window. The only complete solution, as far as I can see, will be allowing the native world to share ownership of the Impl objects with the managed world, via reference counting.