I’ve been spending some time with TSan again recently and it’s flagging up a few things…
The first is that creating juce::TemporaryFiles isn’t safe to do concurrently and boils down to the use of Random::getSystemRandom() in the constructor.
Would it be possible to just use local Random objects here to avoid the data race on the system one?
Could you not just wrap the non thread-safe TemporaryFile into a Decorator Pattern version of it ?
Where your concrete component would simply be an instance of TemporaryFile inside the decorator?
(Then when constructing the decorator, you would make thread-safe the temporary file underlying object construction)
Making this thread-safe would come with a performance penalty for those who don’t need it, so making a decorated thread-safe version of it might make sense?
Hmm, I don’t think adding an extra layer of indirection here is the right approach. Usually I’d agree that thread safety should be left up to the user of a class but in this case the type is pretty fundamental, used internally in a lot of juce classes/function calls and of a type that is extremely often used on background threads.
Reading files, parsing them, creating new ones is a perfect example of the type of thing that should be offloaded to background threads to keep the main thread free and your app responsive.
juce::File::replaceWithData, File::replaceWithText, XmlElement::writeTo, PropertiesFile::saveAsBinary etc. all use juce::TemporaryFile internally. Are you saying that these methods should only be called from a single (probably the message) thread?
Performance wise, these juce::TemporaryFile constructors call the File::getNonexistentChildFile method which includes several disk accesses and system calls plus a ton of memory allocations and string manipulations. Creating a local Random object is unlikely to have a significant performance impact on creating these objects.
If there’s some proof that keeping the few cycles makes a noticeable difference to a program that is creating and writing lots of files there’s other ways to make this thread-safe without passing on responsibility to the caller or creating a local Random object. For example having a static function with a static Random object and a static spin lock would suffice and almost always be just the cost of two atomic stores as the contention is likely to be low.
I think the decorator pattern is a really poor choice for solving this problem, not only because every consumer of the library would have to implement it, they’d also have to copy some internal functions of juce::TemporaryFile, what happens then if the internal behaviour of juce::TemporaryFile changes? Every user of the library has to notice this and copy the internals again just to make sure their own special thread-safe version works the same way as the library implementation?
My fix was literally removing a few characters (::getSystemRandom) in two places in the file and has an absolutely tiny performance hit in a non-performance critical piece of code. Let’s think about this before over-engineering an alternative solution.
i think it will be a good idea, or we make system random thread safe (but that might affect performance in some cases).
