zompi2 / UE4EnhancedCodeFlow

This code plugin for Unreal Engine provides functions that drastically improve the quality of life during the implementation of game flow in C++.
MIT License
94 stars 12 forks source link

UE 5.3, Most Recent Update, Any GC causes a freeze, just this once I caught a crash as well confirming my sus its the plugin. #56

Open nonlin opened 1 month ago

nonlin commented 1 month ago

Using the code from the Unreal Marketplace, saw an update and did it and now I got this strange behavior.

I think prior to the update it was ok, might look at fetching and using older code from here.

Basically it seems like once I start using a coroutine and do "WaitForSeconds" once Unreal decides to do Garbage Collection it would freeze the editor. Somehow I finally got Rider to report a crash (many tests not even Rider would catch a crash, editor would just be frozen and windows didn't even think it stopped responding, but wouldn't even close).

image

zompi2 commented 1 month ago

Thanks for reporting, I will look into it as soon as I can.

zompi2 commented 1 month ago

Sadly, I'm not able to reproduce neither the freeze nor the crash.

I was trying to just run WaitForSeconds coroutine and forcing GC and I was trying to run WaitForSeconds and destroy the calling Actor before the time is up, forcing GC on it, but still the ECF handles everything gracefully.

I found a small issue, probably not related to this problem, but I've pushed some fixes to the version 3.3.9. They are already on github, UE Marketplace will take few hours to process it. I have no faith that this is it, but maybe...

Otherwise I'd need an example of how you use WaitForSeconds and maybe then I will be able to reproduce the issue. It might be because of some more complicated flow I'm not able to imagine.

nonlin commented 1 month ago

More context, I literally only use the coroutine once in one way, this is the way.

FECFCoroutine ACustomGameMode::SpawnNotesForBar_Implementation(TArray<FNotes> Bar, float ExpectedHitTime)
{
    for (int i = 0; i < Bar.Num(); i++)
    {
        if (Bar[i].bLeft)
        {
            SpawnArrow(0, ExpectedHitTime);
        }
        if (Bar[i].bDown)
        {
            SpawnArrow(3);
        }
        if (Bar[i].bUp)
        {
            SpawnArrow(2, ExpectedHitTime);
        }
        if (Bar[i].bRight)
        {
            SpawnArrow(1, ExpectedHitTime);
        }
        co_await FFlow::WaitSeconds(this, FMath::Max((SecondsPerBeat / Bar.Num()) - this->GetWorld()->GetDeltaSeconds(), 0.0f));
    }
}
nonlin commented 1 month ago

Also note the last comment here. Seems related but can't see me not having a UObject without the UPROEPRTY macro on it.

https://github.com/landelare/ue5coro/issues/26#issuecomment-2212461302

zompi2 commented 1 month ago

Ok, I was able to reproduce it. I just had to run multiple co_awaits in one coroutine and run GC.

The problem has appeared in the update 3.3.6 where I was trying to resolve the potential problem of dangling coroutine handles, which might occur if the coroutine has never being resumed. Unfortunatelly, explicit destroy on coroutine handle lead to broken heap allocation, which cause infinite loop or crash after Garbage Collector was called.

I reverted changes from 3.3.6 and let coroutine handles clean itself after the coroutine function. The only moment the handle will not clean itself is if it will never be resumed (for example, because the calling Actor has been destroyed before the coroutine resume). But it's an edge case and the potential memory leak will not be so big. I don't know how to handle such situations properly too.

The version on Marketplace will be live in few hours.

zompi2 commented 1 month ago

Just writing down to not forget: The idea of how to handle it:

  1. Add a flag to coroutine promise informing the coroutine has been finished (the best place will be in return_void)
  2. In the awaiter destructor check if it has: I. invalid owner II. valid coroutine handle III. coro handle has not been finished. If not - mark as finished and destroy it explicitly.
  3. The final_suspend should stay as suspend_never
zompi2 commented 1 month ago

The version 3.3.11 with fixed coroutine handles clearups is on the Marketplace. @nonlin - can you confirm if everything is ok, so I could close this issue?