ReactiveCocoa / ReactiveObjC

The 2.x ReactiveCocoa Objective-C API: Streams of values over time
MIT License
2.6k stars 496 forks source link

Fixed a race condition that was flagged by Xcode's thread sanitizer #161

Closed gotow closed 5 years ago

StatusReport commented 5 years ago

The _mutex protects _inlineDisposables and _disposables. While dealloc is guaranteed to not execute concurrently with other instance methods, for this code to run correctly there must be a memory barrier before the dealloc that ensures that changes to these fields are visible to other cores. Although there's probably some kind of an (implicit?) memory barrier, TSan may complain as these fields are being accessed without _mutex being held.

Note that I'd be very careful to add a mutex here as it may dramatically affect the performance of RAC since in a common usage of this framework RACCompoundDisposable objects are created and destroyed in masses.

mdiep commented 5 years ago

While dealloc is guaranteed to not execute concurrently with other instance methods

Right, so the lock shouldn't be needed.

for this code to run correctly there must be a memory barrier before the dealloc that ensures that changes to these fields are visible to other cores.

I don't follow this

gotow commented 5 years ago

Sorry Matt. After testing again with the thread sanitizer turned on, I can't reproduce the original problem. I'll go back and see if I can figure out how I was generating the errors in the first place (they definitely happened - I even re-checked before submitting my pull request). I agree - the lock shouldn't be needed, but I was getting errors without it. Now I'm not sure why :-/

StatusReport commented 5 years ago

@mdiep it's incorrect to access a shared variable that is being read and written from multiple threads without any memory synchronization mechanism. In this case, a shared variable is being accessed while a lock is being held, but in the dealloc it is accessed without holding the lock.

While this seems like something that should work, as the unprotected access happens on a single thread after the potential writes caused by the instance methods, instruction reordering (which is done by the compiler) and memory reordering (which is done in the CPU) may affect the order that updates to this variable are seen to other threads in the system.

So although instruction reordering is probably irrelevant here, as ObjC is a dynamic language and the compiler cannot reorder instructions between two instance methods, memory reordering is something that can happen as long as there's something that triggers a memory barrier before -dealloc is called.

You can read more about it here and here.