String & thread-safety

Is the String class thread-safe?

I was assuming so, but testing that with the following code it seems it isn’t?

#pragma once

#include "JuceHeader.h"

class StringModifierThread  : public Thread
                            , public ChangeBroadcaster
{
public:
    StringModifierThread() : Thread ("Thread")
    {
        startThread();
    }

    ~StringModifierThread()
    {
        stopThread (1000);
    }

    void run() override
    {
        while (! threadShouldExit())
        {
            string = "abcdef";
            sendChangeMessage();
        }
    }

    String getString() const noexcept   {  return string;   }
    String string;
};

//==============================================================================
class MainContentComponent   : public Component, public ChangeListener
{
public:
    MainContentComponent()
    {
        stringModifier.addChangeListener (this);
        setSize (400, 400);
    }

    void changeListenerCallback (ChangeBroadcaster*) override
    {
        stringModifier.getString();
    }

private:
    StringModifierThread stringModifier;
};

edit: fixed/simplified the code

Well… assignments and copy methods are safe, so a copy of a string can be safely passed between threads. But what you’re doing here is using a reference to the same String object on two threads at the same time, and that’s definitely not safe. If you were to take a local copy, e.g.

    label.setText (String (stringModifier.string), dontSendNotification);

then my guess is that it’d be OK.

right! but I have tried that and I have the same issue.
I was doing it that way actually :

in StringModifierThread :

String getString() const noexcept    {        return string;    }

and in the change callback :
label.setText (stringModifier.getString(), dontSendNotification);

but that won’t work

#0 0x00007fffcd4c9d42 in __pthread_kill ()
#1 0x00000001014237a0 in pthread_kill ()
#2 0x00007fffcd42f420 in abort ()
#3 0x00007fffcd52ed98 in nanozone_error ()
#4 0x00007fffcd524588 in _nano_malloc_check_clear ()
#5 0x00007fffcd5243bf in nano_malloc ()
#6 0x00007fffcd51d282 in malloc_zone_malloc ()
#7 0x00007fffcd51c200 in malloc ()
#8 0x00007fffcbfa9e0e in operator new(unsigned long) ()
#9 0x000000010017b85f in juce::StringHolder::createUninitialisedBytes(unsigned long) at /modules/juce_core/text/juce_String.cpp:71
#10 0x000000010017bbe6 in juce::CharPointer_UTF8 juce::StringHolder::createFromCharPointerjuce::CharPointer_ASCII(juce::CharPointer_ASCII) at /modules/juce_core/text/juce_String.cpp:84
#11 0x000000010017baf5 in juce::String::String(char const*) at /modules/juce_core/text/juce_String.cpp:302
#12 0x000000010014f06d in juce::String::String(char const*) at /modules/juce_core/text/juce_String.cpp:303
#13 0x00000001000028fc in StringModifierThread::run() at /GuiTest/Source/MainComponent.h:29
#14 0x0000000100189163 in juce::thread::threadEntryPoint() at /modules/juce_core/threads/juce_Thread.cpp:96
#15 0x0000000100189545 in juce::juce_threadEntryPoint(void*) at /modules/juce_core/threads/juce_Thread.cpp:111

Are you saying:

void thread()
{
while (1)
   myString = createRandomString();
}

String getString() const { return myString; } 

Calling getString() is completely safe with no further synchronisation?

you don’t even need that “createRandomString()” to get into trouble actually. This will fail already :

    void run() override
    {
        while (! threadShouldExit())
            string = "abcdef";
    }

    String getString() const noexcept   {  return string;   } 

it seems that the underlying ref-count isnt fully thread safe?

1 Like

any other thought about those String copy & thread safety concerns?
should we just consider it as not safe?

You know… I’d really like to say it’s not thread-safe.

That’s not because I don’t want it to be thread-safe, but because if people are relying on it to be safe, then it means we can never safely do the short-string optimisation (or change it to use std::string internally) without adding a mutex, which would kill performance.

The awkward thing is that it’s already pretty-much safe, so that people may be relying on that in some situations, so for us to make it less safe could silently cause problems…