ExecFuture performs blocking reads on Windows, causing deadlocks with `arc lint`
Open, Needs TriagePublic

Description

I'm experiencing hangs whenever arc lint spawns a subprocess that outputs a lot of data to stderr, but not stdout. Cpplint is one such example. Doing a bit of digging, it appears I'm seeing a deadlock where the subprocess is blocked trying to write to stderr, and arcanist is blocked waiting for data on the subprocess' stdout.

The code responsible for spawning the subprocess and reading its stdout/stderr lives in https://secure.phabricator.com/diffusion/PHU/browse/master/src/future/exec/ExecFuture.php. On Linux, it performs non-blocking reads of the subprocess' stdout/stderr streams, but on Windows it uses blocking reads, apparently because PHP can't do non-blocking reads from a pipe on Windows, nor can it determine whether a call to read() on the pipe will block or not (stream_select() doesn't work on pipes on Windows.)

There seems to be some code that uses temporary files for stderr and stdout, ($useWindowsFileStreams) but this is never executed -- a cursory grep of the Phabricator codebase shows that nobody is calling setUseWindowsFileStreams(). Patching ExecFuture so that $useWindowsFileStreams is true fixes the issue.

Version information:
Git 2.6.4
Arcanist and libphutil are the stable branches as of 2016 Week 21.
Windows 7
PHP 5.6
Using cmd.exe terminal.

nickhutchinson renamed this task from ExecFuture performs blocking reads on Windows, causing deadlocks with with `arc lint` to ExecFuture performs blocking reads on Windows, causing deadlocks with `arc lint.
nickhutchinson renamed this task from ExecFuture performs blocking reads on Windows, causing deadlocks with `arc lint to ExecFuture performs blocking reads on Windows, causing deadlocks with `arc lint`.

This is solved with D11688, but it needs to be turned on globally (at the moment it's a per call option, which was added so that internally our custom unit test runner wouldn't deadlock).

This is solved with D11688, but it needs to be turned on globally (at the moment it's a per call option, which was added so that internally our custom unit test runner wouldn't deadlock).

Is there a reason it isn't on by default? How do I turn it on globally without patching the source?

epriestley moved this task from Backlog to Windows on the Arcanist board.Jul 21 2016, 11:41 AM