Rare assertion failure caused by loading a texture for a credit/attribution image while inside the garbage collector

[kring]

(comments below copied from #1132)

I saw this happen once, but it's a rare enough race condition that I couldn't cause it to happen again...

The assertion is "Unable to create new object: %s %s.%s. Creating UObjects while Collecting Garbage is not allowed!" The %s in this case is a UTexture2D. When I saw it happen, the situation was as follows:

1. A level was being unloaded, so the garbage collector was running.
2. The GC called IsReadyForFinishDestroy on an ACesium3DTileset.
3. This method is designed to return true only after all of the tileset's background processing has shut down. So to do that, it ticks the asset accessor and dispatches main thread tasks.
4. Here's where the race condition comes in. In the case where I saw the assertion, ticking the asset accessor just happens to cause an HTTP request for a credit image to complete. Unreal's HttpManager raises the OnProcessRequestComplete callback.
5. The credit system attempts to create a texture for the credit image that was just downloaded.
6. Assertion fails because creating a texture during garbage collection is not allowed.

So that's some pretty unlucky timing, but a clear bug. How to fix it, though?

Perhaps the credit system could dispatch a separate task to the game thread to create the texture, rather than doing it right on the OnProcessRequestComplete callback. That way the stack can unwind out of the GC. It can even call IsGarbageCollecting() and only do this if it is. But need to be careful because in some cases that game thread task may end up running after the level that initiated it has been completely unloaded, so we'd just be swapping one race condition for another.

Or maybe the answer is to dispatch a game thread task to do the tick/dispatch, rather than doing it right from IsReadyForFinishDestroy? As long as that method also returns false, we should be able to rely on that game thread task running before any objects we depend on are destroyed.

Worst case scenario, we could just skip creating credit textures entirely when we happen to be running in GC. 😬

[csciguy8]

From the lessons learned from #1132, my first thoughts are:

• In IsReadyForFinishDestroy, don't make any calls that create a new HTTP request or a new texture. In theory we should be deleting this object, not creating anything new, right?
• Move any shutdown logic that can create garbage collector work to ::EndPlay
• Even though this is a rare case, these notes appear to be good enough to see the implementation problem and fix it.

[kring]

In IsReadyForFinishDestroy, don't make any calls that create a new HTTP request or a new texture. In theory we should be deleting this object, not creating anything new, right?

We're not creating any new HTTP requests in this scenario, but when async requests / operations are already in-flight, we have to wait to destroy the object until those in-flight operations complete. And async operations will never complete (in some cases) if we don't tick the asset accessor and dispatch main thread messages. And dispatching main thread tasks can sometimes (as in this case) create a new texture.

Move any shutdown logic that can create garbage collector work to ::EndPlay

EndPlay only gets called in games and play-in-editor, though. In the case described here, I believe I was exclusively in pure Editor mode.

In any case, we'd need some way to do an async EndPlay. Start ending play, then wait for background tasks to complete, and then finish ending play. I don't know if that's possible. But UE does have a mechanism for that at destruction time (IsReadyForFinishDestroy).

Even though this is a rare case, these notes appear to be good enough to see the implementation problem and fix it.

Yeah I think I might try this approach first: "Or maybe the answer is to dispatch a game thread task to do the tick/dispatch, rather than doing it right from IsReadyForFinishDestroy?" As long as game thread tasks are still dispatched while waiting for IsReadyForFinishDestroy to return true, it should work well. Open to other ideas, though.