I encountered some unexpected behaviors when I was using the timer objects by FTimerManager.
I already spotted the problem and I was able to replicate it in a very simple exampling class:
#pragma once
#include "GameFramework/Actor.h"
#include "Engine/World.h"
#include "TimerManager.h"
#include "Test.generated.h"
class Actor_B;
UCLASS(NotBlueprintable, HideDropdown)
class AActor_A : public AActor
{
GENERATED_BODY()
public:
void SetActor_B(AActor_B* InActor_B) { Actor_B = InActor_B; }
void Foo();
private:
float var_A { 0.0f };
FTimerHandle Timer_A{};
AActor_B* Actor_B{};
};
UCLASS(Blueprintable, BlueprintType)
class AActor_B : public AActor
{
GENERATED_BODY()
public:
AActor_B(const FObjectInitializer& InObjectInitializer)
{
RootComponent = CreateDefaultSubobject<USceneComponent>(TEXT("SceneComponent_B"));
}
virtual void BeginPlay() override
{
Super::BeginPlay();
Actor_A = GetWorld()->SpawnActor<AActor_A>();
Actor_A->SetActor_B(this);
auto lambda_B = [this]()
{
auto* thisCopy = this;
auto* Actor_ACopy = Actor_A;
Actor_A->Foo();
ensureAlways(this == thisCopy);
var_B = 5;
ensureAlways(thisCopy->var_B == 5);
};
GetWorld()->GetTimerManager().SetTimer(Timer_B, std::move(lambda_B), 10.0f, false);
}
private:
int32 var_B{ 0 };
FTimerHandle Timer_B{};
AActor_A* Actor_A{};
public:
void ClearTimer() { GetWorld()->GetTimerManager().ClearTimer(Timer_B); }
};
void AActor_A::Foo()
{
Actor_B->ClearTimer();
auto lambda_A = [this]() {};
GetWorld()->GetTimerManager().SetTimer(Timer_A, std::move(lambda_A), 3.0f, false);
}
In the example above we have AActor_A and AActor_B classes.
I will try to explain briefly what happens:
- the *AActor_B *spawns a instance of AActor_A;
- the *AActor_B *creates a new Timer_B that will be execute the lambda_B lambda function;
- after the Timer_B has expired, during the execution of lambda_B, AActor_A::Foo() function is called;
- AActor_A::Foo() clears the timer member of AActor_B;
- AActor_A::Foo() creates a new timer Timer_A that will execute a lambda_A function;
- *lambda_A *lambda function captures the this of AActor_A instance;
- when AActor_A::Foo() has finished to execute then the execution of lambda_B continues;
- the this captured in lambda_B is changed now! Indeed the ensure macro call (ensureAlways(this == thisCopy)) is always triggered!
At this point the execution of lambda_B is very dangerous since some bad memory access could occur. For example the assignment var_B = 5 is not executed how we expected, in fact that value is written in var_A private member of the AActor_A instance!
I suppose this could be a misusing of timer and a timer should be not cleared while its lambda is executing but I think it’s not so unlikely encounter a similar case and it could occur without our realizing it.
Indeed, in a more complex scenario, lambda_B would have been able trigger a chain of events where we can observe a similar issue.
Checking the FTimerManager implementation I noticed this case is handled, but the timer is removed immediately:
void FTimerManager::InternalClearTimer(FTimerHandle InHandle)
{
SCOPE_CYCLE_COUNTER(STAT_ClearTimer);
// not currently threadsafe
check(IsInGameThread());
FTimerData& Data = GetTimer(InHandle);
switch (Data.Status)
{
case ETimerStatus::Pending:
{
int32 NumRemoved = PendingTimerSet.Remove(InHandle);
check(NumRemoved == 1);
RemoveTimer(InHandle);
}
break;
case ETimerStatus::Active:
Data.Status = ETimerStatus::ActivePendingRemoval;
break;
case ETimerStatus::ActivePendingRemoval:
// Already removed
break;
case ETimerStatus::Paused:
{
int32 NumRemoved = PausedTimerSet.Remove(InHandle);
check(NumRemoved == 1);
RemoveTimer(InHandle);
}
break;
case ETimerStatus::Executing:
check(CurrentlyExecutingTimer == InHandle);
// Edge case. We're currently handling this timer when it got cleared. Clear it to prevent it firing again
// in case it was scheduled to fire multiple times.
CurrentlyExecutingTimer.Invalidate();
RemoveTimer(InHandle);
break;
default:
check(false);
}
}
I think the timer could be set into a new state, we could call it ETimerStatus::ExecutingPendingRemoval, and it will be cleared as soon as possible.
Is it a bad idea?
