vapor / core

🌎 Utility package containing tools for byte manipulation, Codable, OS APIs, and debugging.
MIT License
82 stars 50 forks source link

Process.asyncExecute has threading issue #176

Open weissi opened 6 years ago

weissi commented 6 years ago

Process.asyncExecute contains this code:

https://github.com/vapor/core/blob/96ce86ebf9198328795c4b9cb711489460be083c/Sources/Core/Process%2BExecute.swift#L99-L117

In particular, there's an unsynchronised, shared variable running that's read and written to from multiple threads. That is undefined behaviour which might amongst random crashes, infinite while running loops (even if running gets set to false) might lead to arbitrary worse behaviour.

This can easily be fixed by using a lock around every use of running. But I'd recommend restructuring the code. At present it keeps three background threads blocked for every execution of a process. Worse, because Dispatch normally uses a finitely sized thread-pool this code might just randomly deadlock. The reason it might randomly deadlock is that it will only finish when this code is executed:

https://github.com/vapor/core/blob/96ce86ebf9198328795c4b9cb711489460be083c/Sources/Core/Process%2BExecute.swift#L135-L139

But the promise.succeed is inside of a globalQueue.async which needs an available dispatch thread to run. And while it waits for that dispatch thread to become available it blocks to others (to read stderr and stdout). In other words: We need three available dispatch threads to make progress which is an anti-pattern in Dispatch because the thread pool has a finite number of threads so it can totally happen that all of Dispatch's threads are blocked and only become unblocked when more threads become available (which won't happen if the thread pool is exhausted).