Race Condition Warning in FHttpManager

We’re seeing intermittent hits of this warning in FHttpManager when adding requests from non-game threads:

https://github.com/EpicGames/UnrealEngine/blob/release/Engine/Source/Runtime/Online/HTTP/Private/HttpManager.cpp\#L570

It appears that the flush and add methods are already thread safe, and that bFlushing is no longer a valid sanity check since this change

https://github.com/EpicGames/UnrealEngine/commit/d1b0478e2fd11c75c637e3ca7a7aa3ec5485bbee

This is evidenced by the subsequent downgrade of the check to a warning:

https://github.com/EpicGames/UnrealEngine/commit/03dd6136ed7719d1851df00939cd525d077f483d

In this case, does this need to be a warning at all, or could it be just removed instead?

Thanks!

[Attachment Removed]

Steps to Reproduce[Attachment Removed]

Hi Sean,

Flush is intended to be only used when shutdown to finish remaining task, or something happened we really need to fully clear the remaining http requests to report. It’s a blocking call, not async. If the http requests keep running more than FlushTimeSoftLimitSeconds, they will get cancelled, and if more than FlushTimeHardLimitSeconds, the flow will ignore that and continue.

At that point, it’s not recommended to add further request to increase the flushing time, to block shutdown or report further. So the warning is intended to remind that.

Or sometimes there is mis-usage of Flush. for example, if at some point we really want to wait specific request with blocking call to complete before continuing, ProcessRequestUntilComplete could be used instead of Flush. You can find more usage cases at Engine\Source\Programs\WebTests\Private\TestHttp.cpp

Is it something you think can be resolved on higher level? If not, could you elaborate your use case?

Thank you,

Lorry

[Attachment Removed]

Hi Sean,

Thanks for the info. Currently we use FHttpManager::Flush when shutdown the module(when quitting the game, because Module is relatively low level and usually the last ones to shutdown. ) or when report before crashing the game, so we’re sure no new request will be added at that point.

For your info, there are also other alternatives if you want to wait http quests to complete before continue running the game:

-Create specific type of http requests through a instance of FHttpRetrySystem::FManager, and use FHttpRetrySystem::FManager::BlockUntilFlushed to make sure that type of http requests in that manager are all completed before proceeding;

-Make use of HttpModule->GetHttpManager().SetRequestAddedDelegate and HttpModule->GetHttpManager().SetRequestCompletedDelegate to monitor OngoingRequests, and wait for that to become 0 before proceeding, instead of using Flush;

-Keep the ptrs of created http requests on your own, until IHttpRequest::GetStatus of all kept requests become Failed/Succeeded.

There should be no warning and wait time is unlimited with these alternatives.

I’ll mark this ticket as resolved but don’t hesitate to reply again if you need any further report in the future.

Cheers,

Lorry

[Attachment Removed]

Hi Lorry

Thanks for the reply. I can describe the scenario that brought this up.

We have a plugin that sends telemetry off to some internal systems. It is implemented as domain specific objects getting put into a queue (from any thread) and then having a specific thread that just takes those objects, turns them into http requests, and sends them off. This telemetry system is also a studio telemetry provider, but did not have its flush implemented all the way down to this worker on a separate thread.

When we upgraded to 5.7 and studio telemetry started sending session end events, this led to a high chance of hitting the warning, where we’d receive the session end event and put it in the queue, trigger a flush (which did basically nothing for us), and after that the http manager flush and our separate worker thread putting the http requests could collide. For us, this would manifest during the cook, failing our CI.

Regardless of the warning, we fixed the flush to make sure we didn’t drop the session end event. However, fixing our plugin’s flush doesn’t really stop the collision entirely, as other telemetry sources might send something after that, while http manager’s flush is still going on. We dont have an actual case where that’s happening, it was just a theoretical problem that led to this line of questioning.

I can see the logic of what you explained. If it becomes an actual practical problem for us, we will look to address the problem at a higher level, and if we run into problems can reach out again. Thanks!

[Attachment Removed]