From 093dbc7df1c9084e7ec71473c7119db35353fbfe Mon Sep 17 00:00:00 2001 From: reuk Date: Tue, 18 Jan 2022 16:50:16 +0000 Subject: [PATCH] Plugin Scanning: Fix thread sanitizer issues in the AudioPluginHost --- .../scanning/juce_PluginDirectoryScanner.h | 2 +- .../scanning/juce_PluginListComponent.cpp | 8 +- .../juce_core/files/juce_TemporaryFile.cpp | 21 +++- .../juce_core/native/juce_posix_NamedPipe.cpp | 112 ++++++++++++------ .../juce_ConnectedChildProcess.cpp | 14 ++- 5 files changed, 109 insertions(+), 48 deletions(-) diff --git a/modules/juce_audio_processors/scanning/juce_PluginDirectoryScanner.h b/modules/juce_audio_processors/scanning/juce_PluginDirectoryScanner.h index 7da639b1ea..fc8aa8c506 100644 --- a/modules/juce_audio_processors/scanning/juce_PluginDirectoryScanner.h +++ b/modules/juce_audio_processors/scanning/juce_PluginDirectoryScanner.h @@ -126,7 +126,7 @@ private: File deadMansPedalFile; StringArray failedFiles; Atomic nextIndex; - float progress = 0; + std::atomic progress { 0.0f }; const bool allowAsync; void updateProgress(); diff --git a/modules/juce_audio_processors/scanning/juce_PluginListComponent.cpp b/modules/juce_audio_processors/scanning/juce_PluginListComponent.cpp index b56ea63866..b1e3484a2d 100644 --- a/modules/juce_audio_processors/scanning/juce_PluginListComponent.cpp +++ b/modules/juce_audio_processors/scanning/juce_PluginListComponent.cpp @@ -451,7 +451,8 @@ private: String pluginBeingScanned; double progress = 0; const int numThreads; - bool allowAsync, finished = false, timerReentrancyCheck = false; + bool allowAsync, timerReentrancyCheck = false; + std::atomic finished { false }; std::unique_ptr pool; std::set initiallyBlacklistedFiles; @@ -582,6 +583,8 @@ private: if (timerReentrancyCheck) return; + progress = scanner->getProgress(); + if (pool == nullptr) { const ScopedValueSetter setter (timerReentrancyCheck, true); @@ -602,10 +605,7 @@ private: bool doNextScan() { if (scanner->scanNextFile (true, pluginBeingScanned)) - { - progress = scanner->getProgress(); return true; - } finished = true; return false; diff --git a/modules/juce_core/files/juce_TemporaryFile.cpp b/modules/juce_core/files/juce_TemporaryFile.cpp index 4cf2f9f332..eda1edcdd0 100644 --- a/modules/juce_core/files/juce_TemporaryFile.cpp +++ b/modules/juce_core/files/juce_TemporaryFile.cpp @@ -23,6 +23,23 @@ namespace juce { +// Using Random::getSystemRandom() can be a bit dangerous in multithreaded contexts! +class LockedRandom +{ +public: + int nextInt() + { + const ScopedLock lock (mutex); + return random.nextInt(); + } + +private: + CriticalSection mutex; + Random random; +}; + +static LockedRandom lockedRandom; + static File createTempFile (const File& parentDirectory, String name, const String& suffix, int optionFlags) { @@ -34,7 +51,7 @@ static File createTempFile (const File& parentDirectory, String name, TemporaryFile::TemporaryFile (const String& suffix, const int optionFlags) : temporaryFile (createTempFile (File::getSpecialLocation (File::tempDirectory), - "temp_" + String::toHexString (Random::getSystemRandom().nextInt()), + "temp_" + String::toHexString (lockedRandom.nextInt()), suffix, optionFlags)), targetFile() { @@ -43,7 +60,7 @@ TemporaryFile::TemporaryFile (const String& suffix, const int optionFlags) TemporaryFile::TemporaryFile (const File& target, const int optionFlags) : temporaryFile (createTempFile (target.getParentDirectory(), target.getFileNameWithoutExtension() - + "_temp" + String::toHexString (Random::getSystemRandom().nextInt()), + + "_temp" + String::toHexString (lockedRandom.nextInt()), target.getFileExtension(), optionFlags)), targetFile (target) { diff --git a/modules/juce_core/native/juce_posix_NamedPipe.cpp b/modules/juce_core/native/juce_posix_NamedPipe.cpp index 5af501ce6a..0e6e678324 100644 --- a/modules/juce_core/native/juce_posix_NamedPipe.cpp +++ b/modules/juce_core/native/juce_posix_NamedPipe.cpp @@ -39,8 +39,8 @@ public: ~Pimpl() { - if (pipeIn != -1) ::close (pipeIn); - if (pipeOut != -1) ::close (pipeOut); + pipeIn .close(); + pipeOut.close(); if (createdPipe) { @@ -51,7 +51,7 @@ public: bool connect (int timeOutMilliseconds) { - return openPipe (true, getTimeoutEnd (timeOutMilliseconds)); + return openPipe (true, getTimeoutEnd (timeOutMilliseconds)) != invalidPipe; } int read (char* destBuffer, int maxBytesToRead, int timeOutMilliseconds) @@ -61,8 +61,10 @@ public: while (bytesRead < maxBytesToRead) { + const auto pipe = pipeIn.get(); + auto bytesThisTime = maxBytesToRead - bytesRead; - auto numRead = (int) ::read (pipeIn, destBuffer, (size_t) bytesThisTime); + auto numRead = (int) ::read (pipe, destBuffer, (size_t) bytesThisTime); if (numRead <= 0) { @@ -72,9 +74,9 @@ public: return -1; const int maxWaitingTime = 30; - waitForInput (pipeIn, timeoutEnd == 0 ? maxWaitingTime - : jmin (maxWaitingTime, - (int) (timeoutEnd - Time::getMillisecondCounter()))); + waitForInput (pipe, timeoutEnd == 0 ? maxWaitingTime + : jmin (maxWaitingTime, + (int) (timeoutEnd - Time::getMillisecondCounter()))); continue; } @@ -89,7 +91,9 @@ public: { auto timeoutEnd = getTimeoutEnd (timeOutMilliseconds); - if (! openPipe (false, timeoutEnd)) + const auto pipe = openPipe (false, timeoutEnd); + + if (pipe == invalidPipe) return -1; int bytesWritten = 0; @@ -97,7 +101,7 @@ public: while (bytesWritten < numBytesToWrite && ! hasExpired (timeoutEnd)) { auto bytesThisTime = numBytesToWrite - bytesWritten; - auto numWritten = (int) ::write (pipeOut, sourceBuffer, (size_t) bytesThisTime); + auto numWritten = (int) ::write (pipe, sourceBuffer, (size_t) bytesThisTime); if (numWritten < 0) { @@ -105,9 +109,9 @@ public: const int maxWaitingTime = 30; if (error == EWOULDBLOCK || error == EAGAIN) - waitToWrite (pipeOut, timeoutEnd == 0 ? maxWaitingTime - : jmin (maxWaitingTime, - (int) (timeoutEnd - Time::getMillisecondCounter()))); + waitToWrite (pipe, timeoutEnd == 0 ? maxWaitingTime + : jmin (maxWaitingTime, + (int) (timeoutEnd - Time::getMillisecondCounter()))); else return -1; @@ -134,8 +138,52 @@ public: return createdFifoIn && createdFifoOut; } + static constexpr auto invalidPipe = -1; + + class PipeDescriptor + { + public: + template + int get (Fn&& fn) + { + { + const ScopedReadLock l (mutex); + + if (descriptor != invalidPipe) + return descriptor; + } + + const ScopedWriteLock l (mutex); + return descriptor = fn(); + } + + void close() + { + { + const ScopedReadLock l (mutex); + + if (descriptor == invalidPipe) + return; + } + + const ScopedWriteLock l (mutex); + ::close (descriptor); + descriptor = invalidPipe; + } + + int get() + { + const ScopedReadLock l (mutex); + return descriptor; + } + + private: + ReadWriteLock mutex; + int descriptor = invalidPipe; + }; + const String pipeInName, pipeOutName; - int pipeIn = -1, pipeOut = -1; + PipeDescriptor pipeIn, pipeOut; bool createdFifoIn = false, createdFifoOut = false; const bool createdPipe; @@ -160,30 +208,25 @@ private: { auto p = ::open (name.toUTF8(), flags); - if (p != -1 || hasExpired (timeoutEnd) || stopReadOperation.load()) + if (p != invalidPipe || hasExpired (timeoutEnd) || stopReadOperation.load()) return p; Thread::sleep (2); } } - bool openPipe (bool isInput, uint32 timeoutEnd) + int openPipe (bool isInput, uint32 timeoutEnd) { auto& pipe = isInput ? pipeIn : pipeOut; - int flags = (isInput ? O_RDWR : O_WRONLY) | O_NONBLOCK; + const auto flags = (isInput ? O_RDWR : O_WRONLY) | O_NONBLOCK; const String& pipeName = isInput ? (createdPipe ? pipeInName : pipeOutName) : (createdPipe ? pipeOutName : pipeInName); - if (pipe == -1) + return pipe.get ([this, &pipeName, &flags, &timeoutEnd] { - pipe = openPipe (pipeName, flags, timeoutEnd); - - if (pipe == -1) - return false; - } - - return true; + return openPipe (pipeName, flags, timeoutEnd); + }); } static void waitForInput (int handle, int timeoutMsecs) noexcept @@ -203,23 +246,18 @@ private: void NamedPipe::close() { - { - ScopedReadLock sl (lock); + ScopedWriteLock sl (lock); - if (pimpl != nullptr) - { - pimpl->stopReadOperation = true; + if (pimpl != nullptr) + { + pimpl->stopReadOperation = true; - char buffer[1] = { 0 }; - ssize_t done = ::write (pimpl->pipeIn, buffer, 1); - ignoreUnused (done); - } + const char buffer[] { 0 }; + const auto done = ::write (pimpl->pipeIn.get(), buffer, numElementsInArray (buffer)); + ignoreUnused (done); } - { - ScopedWriteLock sl (lock); - pimpl.reset(); - } + pimpl.reset(); } bool NamedPipe::openInternal (const String& pipeName, bool createPipe, bool mustNotExist) diff --git a/modules/juce_events/interprocess/juce_ConnectedChildProcess.cpp b/modules/juce_events/interprocess/juce_ConnectedChildProcess.cpp index e97a819094..3623c064c5 100644 --- a/modules/juce_events/interprocess/juce_ConnectedChildProcess.cpp +++ b/modules/juce_events/interprocess/juce_ConnectedChildProcess.cpp @@ -51,6 +51,8 @@ struct ChildProcessPingThread : public Thread, pingReceived(); } + void startPinging() { startThread (4); } + void pingReceived() noexcept { countdown = timeoutMs / 1000 + 1; } void triggerConnectionLostMessage() { triggerAsyncUpdate(); } @@ -90,8 +92,7 @@ struct ChildProcessCoordinator::Connection : public InterprocessConnection, ChildProcessPingThread (timeout), owner (m) { - if (createPipe (pipeName, timeoutMs)) - startThread (4); + createPipe (pipeName, timeoutMs); } ~Connection() override @@ -99,6 +100,8 @@ struct ChildProcessCoordinator::Connection : public InterprocessConnection, stopThread (10000); } + using ChildProcessPingThread::startPinging; + private: void connectionMade() override {} void connectionLost() override { owner.handleConnectionLost(); } @@ -198,7 +201,6 @@ struct ChildProcessWorker::Connection : public InterprocessConnection, owner (p) { connectToPipe (pipeName, timeoutMs); - startThread (4); } ~Connection() override @@ -207,6 +209,8 @@ struct ChildProcessWorker::Connection : public InterprocessConnection, disconnect(); } + using ChildProcessPingThread::startPinging; + private: ChildProcessWorker& owner; @@ -275,7 +279,9 @@ bool ChildProcessWorker::initialiseFromCommandLine (const String& commandLine, { connection.reset (new Connection (*this, pipeName, timeoutMs <= 0 ? defaultTimeoutMs : timeoutMs)); - if (! connection->isConnected()) + if (connection->isConnected()) + connection->startPinging(); + else connection.reset(); } }