search

CesiumGS / cesium-unity

Bringing the 3D geospatial ecosystem to Unity
https://cesium.com/platform/cesium-for-unity/
Apache License 2.0
358 stars 83 forks source link

Fix another crash caused by destroying CesiumIonSession while it is doing network requests #499

Closed kring closed 2 months ago

kring commented 2 months ago

In #479, I made some improvements to CesiumIonSession to 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 refresh methods 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.

kring commented 2 months ago

Merging this for tomorrow's release (it's very low risk).