We’re doing some TStrongObjectPtr stuff across threads, so I took a look at FUObjectItem::AddRef to make sure it was thread safe, and I’m a little worried it’s not?
I see we interlocked add to the ref count, but then without using the previously returned value or checking that the ref count hasn’t changed, we set the RefCounted flag.
What if somebody calls AddRef on one thread and ReleaseRef on another thread very shortly after? couldn’t we see:
1) Thread A: AddRef increments the count
2) Thread B: Release Ref decrements the count
3) Thread B: Release Ref removes the RefCounted flag (doing nothing)
4) Thread A: AddRef sets the RefCounted flag
Unless I’m misunderstanding, this would leave the RefCounted flag set. Is that okay? the RefCount would still be 0, so maybe we verify later that if the flag is set, we have a non-zero RefCount? if that’s the case, is it worth adding a comment?
unless I’m misunderstanding your explanation, every ReleaseRef() should always be preceded by an AddRef() at some point during the lifetime of the program.
The referencer is expected to guarantee that behavior.
So there is an implied step 0 here:
0) Thread B (or any other thread): AddRef() increments the count to >0
…
By including that it should be guaranteed that even if another thread decrements in between, the end result is still greater than zero, which is the minimal required post-condition of this code I’d say (along with the implied consistency between Add/Release).
> I think worst case would be using AddRef and ReleaseRef outside of TStrongObjectPtr
Yes, agree with you there. The “users” of the AddRef/ReleaseRef must take extra care to make sure they are consistent. We’re reasonable sure that TStrongObjectPtr meets that requirement, so please use it over any custom logic!
And of course, if you really find any concrete issues with it please let us know here.
Thanks! Taking a look at TStrongObjectPtr I see it’s always adding the ref before setting the internal value, so you’re right, that should guarantee the ordering. I think worst case would be using AddRef and ReleaseRef outside of TStrongObjectPtr. Imaging two tasks running at the same time, one adds a reference to an object (maybe that is has looked up by name) and another also has a pointer to that object somehow, sees that the reference count is non-zero (because of the first half of the add ref on the other thread) and calls ReleaseRef. Possibly that is too contrived to be worth worrying about. Regardless, it seems as long as we stick to using TStrongObjectPtr as our interface to the reference counting, everything should be fine.