Page MenuHomePhabricator

Support non-blocking reads for ExecFuture on Windows
ClosedPublic

Authored by hach-que on Feb 5 2015, 3:27 AM.

Details

Summary

This enables non-blocking reads on Windows for ExecFuture by redirecting the process's standard output and error to temporary files. We then use stream_select on the open files to determine when there is new data to read.

This is primarily to enable setUpdateInterval to work on FutureIterator when running under Windows, which is essential when performing arc unit --everything with lots of tests.

Test Plan

Ran arc diff and arc unit --everything on a project with lots of tests. Saw everything behave correctly and saw setUpdateInterval work as it does on Linux.

Diff Detail

Repository
rPHU libphutil
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

hach-que updated this revision to Diff 28128.Feb 5 2015, 3:27 AM
hach-que retitled this revision from to Support non-blocking reads for ExecFuture on Windows.
hach-que updated this object.
hach-que edited the test plan for this revision. (Show Details)
hach-que added a reviewer: epriestley.
hach-que updated this revision to Diff 28129.Feb 5 2015, 3:29 AM
hach-que edited edge metadata.
  • Remove commented code
epriestley edited edge metadata.Feb 8 2015, 10:05 PM

Some inlines. Since this mechanism is certain to be fraught with peril, can we reasonably have an ExecFuture opt in to it for now until we're more confident it works properly? Then only your stuff will break if we hit further Windows weirdness. For example, something like setUseWindowsFileStreams(true).

src/future/exec/ExecFuture.php
588–589

This seems completely crazy -- do feof(), fseek(), ftell(), etc., not work? I'd expect them to work properly since these are standard files.

599

$real_length is only written to and never read, so something is awry in any case.

690–691

These objects are local and not preserved, so I would expect that we'll try to destroy the temporary files immediately upon exiting this function, and fail. In net, I'd expect that we will never remove these files. Do these files get removed?

Roughly, I'd expect that you need to save these to $this->something. That might also resolve the unlink issue on its own.

We should make every reasonable effort to unlink these files.

The default permissions of TempFile also aren't as locked-down as they could be, but it seems like these files should be as tightly locked as possible. Not sure if we can reasonably do anything or what chmod() translates to under Windows, though.

695–696

These calls should be tested for errors.

715–716

As above.

hach-que updated this revision to Diff 28194.Feb 8 2015, 10:59 PM
hach-que edited edge metadata.
  • Changes from code review feedback
hach-que updated this revision to Diff 28198.Feb 9 2015, 4:57 AM

Remove stream_select as things appear to work without it

hach-que updated this revision to Diff 28199.Feb 9 2015, 5:34 AM

Uh, don't derp on the arc diff

epriestley accepted this revision.Feb 9 2015, 2:17 PM
epriestley edited edge metadata.
epriestley added inline comments.
src/future/exec/ExecFuture.php
685–686

These variables are unused.

701–702

pht()

721–722

pht()

This revision is now accepted and ready to land.Feb 9 2015, 2:17 PM
hach-que updated this revision to Diff 28281.Feb 11 2015, 1:21 AM
hach-que edited edge metadata.

Update for code review

This revision was automatically updated to reflect the committed changes.