From 21b0cd46639967e1eefb5a82c930eb856b4777ed Mon Sep 17 00:00:00 2001 From: reuk Date: Mon, 30 Jun 2025 18:55:47 +0100 Subject: [PATCH] Network: Fix potential deadlock in macOS WebInputStream Regarding didComplete(): When a WebInputStream is destroyed, the thread of execution will wait in SharedSession::removeTask() until the task is no longer present in the list of all active tasks. If multiple threads are all waiting in removeTask(), then all of those threads should wake when the set of active tasks changes. Waking only a single thread may result in deadlocks, as that thread's task may not have completed successfully. Then, the thread that woke up will be forced to sleep again and may not get another chance to wake. Regarding didBecomeInvalid(): Normally, didBecomeInvalid() will only be called after the SharedSession's destructor. If the destructor is running, we may assume that no other thread can access the SharedSession, so using notify_one() in didBecomeInvalid() should be sufficient to wake up the destructor's thread. However, there's a chance that the NSURLSession may be invalidated unexpectedly (i.e. before the SharedSession's destructor runs), in which case there may still be threads waiting in removeTask(). In this scenario we need to notify_all() so that all waiting threads are able to wake and make progress. --- modules/juce_core/native/juce_Network_mac.mm | 40 +++++++++----------- 1 file changed, 17 insertions(+), 23 deletions(-) diff --git a/modules/juce_core/native/juce_Network_mac.mm b/modules/juce_core/native/juce_Network_mac.mm index dc9f157099..932751c099 100644 --- a/modules/juce_core/native/juce_Network_mac.mm +++ b/modules/juce_core/native/juce_Network_mac.mm @@ -140,15 +140,18 @@ public: ~SharedSession() { std::unique_lock lock { mutex }; - [session.get() finishTasksAndInvalidate]; - condvar.wait (lock, [&] { return state == State::stopped; }); + + if (session != nullptr) + [session.get() finishTasksAndInvalidate]; + + condvar.wait (lock, [&] { return session == nullptr; }); } NSUniquePtr addTask (NSURLRequest* request, SessionListener* listener) { std::unique_lock lock { mutex }; - if (state != State::running) + if (session == nullptr) return nullptr; NSUniquePtr task { [[session.get() dataTaskWithRequest: request] retain] }; @@ -170,28 +173,28 @@ private: DBG (nsStringToJuce ([error description])); #endif - const auto toNotify = [&] + const auto toNotify = std::invoke ([&] { const std::scoped_lock lock { mutex }; - state = State::stopRequested; + session.reset(); // Take a copy of listenerForTask so that we don't need to hold the lock while // iterating through the remaining listeners. - return listenerForTask; - }(); + return std::exchange (listenerForTask, {}); + }); for (const auto& pair : toNotify) pair.second->didComplete (error); - const std::scoped_lock lock { mutex }; - listenerForTask.clear(); - state = State::stopped; - - // Important: we keep the lock held while calling condvar.notify_one(). + // Important: we keep the lock held while calling condvar.notify_all(). // If we don't, then it's possible that when the destructor runs, it will wake // before we notify the condvar on this thread, allowing the destructor to continue // and destroying the condition variable. When didBecomeInvalid resumes, the condition // variable will have been destroyed. - condvar.notify_one(); + // Use notify_all() rather than notify_one() so that all threads waiting + // in removeTask() can make progress in the case that the session is + // invalidated unexpectedly. + const std::scoped_lock lock { mutex }; + condvar.notify_all(); } void didComplete (NSURLSessionTask* task, [[maybe_unused]] NSError* error) @@ -213,7 +216,7 @@ private: listenerForTask.erase ([task taskIdentifier]); } - condvar.notify_one(); + condvar.notify_all(); } void didReceiveResponse (NSURLSessionTask* task, @@ -324,13 +327,6 @@ private: } }; - enum class State - { - running, - stopRequested, - stopped, - }; - std::mutex mutex; std::condition_variable condvar; @@ -343,8 +339,6 @@ private: }; std::map listenerForTask; - - State state = State::running; }; class TaskToken