FObjectTrace::GetObjectId is not thread safe - Invoked from anim graph across multiple threads

Development Programming & Scripting
, ,
1
  • FObjectTrace::GetObjectId can end up allocating different object IDs for the same object if invoked on multiple threads at the same time.
  • This can happen during Parallel animation evaluation when we invoke FBlendStackAnimPlayer::Initialize for the same animation asset
  • This is caused by the fact that there is a .Get() followed by a .Add() as 2 separate operations in FObjectTrace::GetObjectId. It’s possible for multiple threads to pass the condition and invoke the .Add() multiple times.
  • This can also cause several checkSlow() to trigger in the RemoveAnnotation() for FUObjectAnnotationSparseSearchable when the object is garbage collected.

Cheers!

2

Steps to Reproduce

  • Invoke FObjectTrace::GetObjectId on the same object across multiple threads.
  • Notice that the resulting uint64 is not consistent between calls
  • The issue is that we invoke .GetAnnotation(), and then if this hasn’t been added we then invoke .AddAnnotation() with a unique atomically incrementing static. This is not thread safe as 2 threads could both return 0 and both pass the condition, and then we invoke .AddAnnotation() twice for the same object ID with 2 different FObjectIDAnnotation values. This also triggers the checkSlow() in full debug in the object annotation container.
  • Download the sample and simply PIE, it should trip up straight away.
  • This would be fine if we only invoked this on a single thread, but FBlendStackAnimPlayer::Initialize invoked FObjectTrace::GetObjectId potentially across multiple threads on the same UAnimationAsset.
3

Hi, thanks for logging this and for taking the time to put together the repro project. I’ve discussed it with the dev team, and we feel that the best way to fix this is by making a change to FObjectTrace::GetObjectId rather than just working around it within the blend stack code.

Unfortunately, since AddAnnotation replaces an existing annotation when the same object is passed, we can’t just change the return to query GetAnnotation as I’d hoped. So I think instead, we will need to add an extra lock so the code will look something like this:

	auto GetObjectIdInner = [](const UObject* InObjectInner)
	{
		FObjectIdAnnotation Annotation = GObjectIdAnnotations.GetAnnotation(InObjectInner);
		if (Annotation.Id != 0)
		{
			return Annotation.Id;
		}
 
		UE::TScopeLock ScopeLock(GObjectIdAllocationLock);
		Annotation = GObjectIdAnnotations.GetAnnotation(InObjectInner);
		if (Annotation.Id == 0)
		{
			Annotation.Id = AllocateInstanceId();
			GObjectIdAnnotations.AddAnnotation(InObjectInner, Annotation);
		}
		return Annotation.Id;
	};

I’m waiting to get that reviewed by the dev team. I’ll follow up once I have more info, but let me know if you have any further thoughts.

4

[mention removed]​ Did the team have any further changes to the suggested code?

5

Hi, yes, I ended up making some changes to avoid having to take the extra lock. I’ve attached a patch with the updated code. It adds a function to FUObjectAnnotationSparseSearchable, which will return an existing annotation if one exists when attempting to add rather than replacing it as AddAnnotation does. I haven’t committed this yet, but the final fix will at least be along these lines, if not exactly this change.

GetObjectIdConcurrencyFix_Main.zip(1.27 KB)