FTimeSpan::GetSeconds() considered harmful!

Development Programming & Scripting C++
1

Does your game use GetSeconds()? Does your game have weird, intermittent problems? There might be a direct correlation.

FTimespan::GetSeconds() can cause sneaky, intermittent bugs. It is also exposed to Blueprint in the “Math|Timespan” category with the description “Get Seconds”.

The body of this innocuously named function (in 5.3.somethingOrOther), from Timespan.h:

	int32 GetSeconds() const
	{
		return (int32)((Ticks / ETimespan::TicksPerSecond) % 60);
	}

This function gets the number of seconds as would be displayed on a digital clock, not the total number of seconds (which is what the aptly named GetTotalSeconds() is for).

There are quite a few usages of this function throughout Epics own code, and four of them (in my version, yada yada) are deeply suspect, and have probably caused people some serious headaches trying to track their odd behavior down):

\Engine\Plugins\Messaging\UdpMessaging\Source\UdpMessaging\Private\Transport\UdpMessageTransport.cpp:80

		if ((CurrentTime - DenyCandidate.LastFailTime).GetSeconds() < CVarBadEndpointPeriod.GetValueOnAnyThread())

\Engine\Plugins\Runtime\HairStrands\Source\HairStrandsCore\Private\GroomBuilder.cpp:1350

				const double RemainingTimeInSeconds = RemainingTasks * double(ElaspedTime.GetSeconds() / SlowTask.CompletedWork);

\Engine\Plugins\Runtime\HairStrands\Source\HairStrandsCore\Private\HairCardsBuilder.cpp:1523

				const double RemainingTimeInSeconds = RemainingTasks * double(ElaspedTime.GetSeconds() / SlowTask.CompletedWork);

\Engine\Source\Runtime\Networking\Public\Common\TcpListener.h:206

				FPlatformProcess::Sleep(SleepTime.GetSeconds());

Note that I also evaluated the usages of GetMinutes and GetHours, but nothing jumped out at me. GetDays just returns the total days, no such problems there.

Given the problems it can (and has) caused, I recommend marking GetSeconds, GetMinutes, and GetHours as deprecated, and introducing GetClockSeconds, GetClockMinutes, and GetClockHours.

This deprecation will help others find the usages in their own code that may be causing problems and fix them.

2

Interesting.

Can anyone corroborate or contest this?

3

For context, here’s the GitHub links into the relevant files. Note that you’ll be blocked unless you’ve set up your GitHub account properly.

UdpMessageTransport.cpp, line 80

GroomBuilder, Line 1350 (now 1553)

HairCardsBuilder seems to have gone through an overhaul and no longer uses GetSeconds()

TcpListener.h, line 206 (now 192)

None of these seem terribly serious, but are clearly bugs none the less.

TcpListener, for example, calls it on SleepTime, which defaults to 1 second when trying to set how long a given connection will sleep. If someone asked for 60 seconds, and got zero, that’d be pretty blatant, but the default behaves just fine.

UdpMessageTransport’s bug is in HandleEndpointError(). It’s subtracting two FDateTime’s, which yields an FTimeSpan. The bad news is the default value of CVarBadEndpointPeriod is 60 seconds, so that test will ALWAYS return true, resulting in more endpoints being denied than was intended.

4

I found a new instance of this bug in the 5.4 code base, in Engine/Source/Programs/SlateUGS/Private/UGSTab.cpp line 684:

// TODO check this is valid, may be off
	WorkspaceSettings->LastSyncDurationSeconds = (FDateTime::UtcNow() - WorkspaceContext->StartTime).GetSeconds();

Tellingly, it looks like someone went looking for the bug and couldn’t find it. The name GetSeconds is that misleading.

Could someone from Epic Games take a look at his? In our game’s codebase, 15 usages of GetSeconds were correct (including engine code) , and 6 were in error. That’s almost 1 in 4. How many other people’s games have this sort of bug hiding in them too?