Data Race in AudioParameterBool

Thread Sanitizer in xcode finds a data race in AudioParameterBool::setValue() and AudioParameterBool::get()

Would that the race goes away if the private member float value; was atomic?

Yes, but there’s no guarantee on all the platforms that JUCE supports that it would also be lock free. Whether or not that’s worth it has been a discussion that’s been rumbling along on these forums for a very long time.

Refactoring all the audio parameter stuff to get rid of all the sanitizer warnings is on our todo list.

4 Likes

Found one in ReferenceCountedObjectPtr:

    /** Takes-over the object from another pointer. */
    ReferenceCountedObjectPtr& operator= (ReferenceCountedObjectPtr&& other) noexcept
    {
        std::swap (referencedObject, other.referencedObject); //here
        return *this;
    }

called from:
imageToRenderTo[renderIndex] = std::move(image);
with:
std::array<Image, 2> imageToRenderTo;

The Image class is a lightweight wrapper around a referenced counted pointer to the actual image data. You can increment and decrement the reference counts safely from different threads, but replacing a ReferenceCountedObjectPtr with a different ReferenceCountedObjectPtr is not a thread-safe operation.

Doing something like this

{
    Image image (Image::PixelFormat::ARGB, 1, 1, true);
    Thread::launch ([image]() { image.isValid(); });
}

is fine. The reference count of image is increased when it’s captured by value (the actual image data is not copied, so this is a really fast operation), so the thread is able to access the image data. Now both the main thread and the launched thread decrement the reference count of image. If ReferenceCountedObjectPtr was not thread safe this would be a nasty data race that could end up double-deleting the image data.

ok so the move-semantics functions in ReferenceCountedObjectPtr are not thread-safe. but the copy-semantics functions are thread-safe. Got it.

that should probably be noted in the docs for any class built from ReferenceCountedObjectPtr, no?

No, I didn’t say that.

Incrementing and decrementing the reference counts are thread safe. So you could do something like

{
    auto otherImage = image;
}

in the launched thread. This will increment the reference count when the otherImage variable is created, and decrement it again when it goes out of scope. You could also pass otherImage off to a different function (on another different thread, if you like).

It is not thread safe to replace a shared ReferenceCountedObjectPtr with a different ReferenceCountedObjectPtr. It doesn’t matter if you use copy or move assignment.

My point is that the documentation does not indicate that that’s what happens with those move semantics and that’s why it should be noted as such.

I think you might be getting hung up on move semantics. It doesn’t make any difference if you use move assignment or copy assignment. It’s only the reference count that’s atomic, so the only thread safe operation is incrementing or decrementing the reference count.

The docs state that the reference count is atomic. They don’t make any guarantees about anything else. It’s the same as std::shared_ptr - the reference counting is thread safe, but access to the object contained within it is not.

I’m advocating for this in the ReferenceCountedObjectPtr docs:

  • the docs should say that access to the owned pointer is not thread safe
  • the move-semantic functions operate on this pointer directly
  • the move-semantic functions do nothing with the thread-safe reference counter.

Atomic is never mentioned on the ReferenceCountedObjectPtr docs.
I don’t know if this behavior is implied by move semantic conventions, but I stepped in this data race hole, and I’m sure other people have too. I’m just trying to make the tools easier to use correctly.

Then the question is why you thought modifying the pointed-to object would be safe in the first place.

None of JUCE’s other containers or even the standard library containers offer this. They don’t explicitly state this in the docs because this is the default position. You cannot safely concurrently modify shared state. In all container types you can access the contained data and change it.

I’m not sure what you mean. Again, it’s not the fact that it’s a move that’s problematic. It’s because you’re modifying data shared between the two threads. If you used copy assignment instead you would run into exactly the same problem.

Using your original example you have

std::array<Image, 2> imageToRenderTo;

shared between two threads. You can freely make copies of the data contained within each element of imageToRenderTo in any thread

auto x = imageToRenderTo[0];

and you can hand that x off elsewhere in your code. What you cannot do is modify the data in the Images in that array. So if you did something like

Graphics g (imageToRenderTo[0]);
g.drawRect (0, 0, 1, 1);

on the non-message thread, at the same time as drawing the image to the screen on the message thread, then you will have a data race. This is because you’re concurrently modifying and reading the image data that imageToRenderTo[0] points to.

You run into exactly the same problem if you do something like

Image otherImage (Image::PixelFormat::ARGB, 12, 12, true);
imageToRenderTo[0] = otherImage;

or

imageToRenderTo[0] = std::move (otherImage);

for the same reason - because you are modifying the shared internal state of imageToRenderTo[0].

Yes, because you can have a ReferenceCountedObjectPtr managing a SingleThreadedReferenceCountedObject, which is obviously not thread safe…

Don’t get caught up on move semantics. This is about modifying data shared between threads. A copy assignment will cause the same amount of trouble.

if atomic types are the only types that are thread-safe, why don’t container classes that allocate on the heap use them as the internal data type? why isn’t ReferencedType* referencedObject atomic in ReferenceCountedObjectPtr?

Not true, because the ReferenceCountedObjectPtr copy assignment just increments the use count, which is atomic and therefore thread-safe.

ReferenceCountedObjectPtr& ReferenceCountedObjectPtr<ObjectType>::operator=( const ReferenceCountedObjectPtr<ObjectType>& other )	
Changes this pointer to point at a different object.

The reference count of the old object is decremented, and it might be deleted if it hits zero. The new object's count is incremented.

To make a whole object (which ReferencedType is) thread safe, you will have to add locks and critical sections around any function, that accesses member variables. There is no generic paradigm available, that would do that, and if it existed, it would be hard to control anyway, because any interaction would lock and eventually you get even potentially dead locks.

Making things thread safe is usually limited to primitive members (where you can use atomic) or by contract, like processBlock(), where you know processBlock() will never be called in parallel to prepareToPlay() for instance.
Or if it is a bigger data chunk, you will have to lock the access methods, or do atomic swaps of data.

There is no way, that a MemoryHolder or Container guarantees the threadsafety of whatever is contained.

it’s a pointer, not an object.

Atomic<ReferencedType*> referencedObject {nullptr}; should be fine…

what am i missing?
why is that referencedObject pointer a raw pointer in ReferenceCountedObjectPtr?

It’s a pointer to anything, potentially an object (and most likely, given that the ReferencedObject is an intrusive reference counted object…)

It is true, because the copy-constructor doesn’t just increment the use count, it also has to store a pointer to the managed object inside the ReferenceCountedObjectPtr instance. The race in your move-assign example happens because the pointer data member of the ReferenceCountedObjectPtr is being modified, which is also true in the copy assignment case.

   ReferenceCountedObjectPtr (const ReferenceCountedObjectPtr& other) noexcept
        : referencedObject (other.referencedObject)
    {
        incIfNotNull (referencedObject);
    }

that’s not what I’m seeing here… :thinking:

No, it’s true.

Try this:

struct Example
{
    Example()
    {
        Thread::launch ([this]()
        {
            imageToRenderTo[0] = otherImage;
        });

        imageToRenderTo[0].isValid();
    }

    Image otherImage { Image::PixelFormat::ARGB, 9, 9, true };

    std::array<Image, 2> imageToRenderTo
    {
        Image (Image::PixelFormat::ARGB, 12, 12, true),
        Image (Image::PixelFormat::ARGB, 32, 32, true)
    };
};

The thread sanitizer will flag it.

That’s not the assignment operator.

  • That’s the copy constructor, not the copy assignment operator
  • The copy constructor copies other.referencedObject to its own referencedObject data member in the constructor initialiser list

My fault, you’re right, I’ve been looking at the wrong function.

That still doesn’t explain why the single member variable of this class, which is a pointer, isn’t an atomic pointer.