Changeset View
Changeset View
Standalone View
Standalone View
src/future/exec/ExecFuture.php
| Show First 20 Lines • Show All 328 Lines • ▼ Show 20 Lines | /* -( Configuring Execution )---------------------------------------------- */ | ||||
| * be killed and the future will resolve with an error code. You can test | * be killed and the future will resolve with an error code. You can test | ||||
| * if a future was killed by a timeout with @{method:getWasKilledByTimeout}. | * if a future was killed by a timeout with @{method:getWasKilledByTimeout}. | ||||
| * | * | ||||
| * @param int Maximum number of seconds this command may execute for. | * @param int Maximum number of seconds this command may execute for. | ||||
| * @return this | * @return this | ||||
| * @task config | * @task config | ||||
| */ | */ | ||||
| public function setTimeout($seconds) { | public function setTimeout($seconds) { | ||||
| // Timeout requires non-blocking streams | |||||
| $this->setUseWindowsFileStreams(TRUE); | |||||
BYK: This fix should probably be in its own patch but I needed it to get my tests passing. | |||||
Not Done Inline ActionsWe can probably enable these by default now (i.e. turn on Windows file streams by default instead of explicitly turning it on here). We've been using them for ages and they work fine. hach-que: We can probably enable these by default now (i.e. turn on Windows file streams by default… | |||||
Not Done Inline ActionsNot turning them on unless necessary avoid temporary file creation etc. so I'm unsure. That said I'm okay if you think that's a good idea. BYK: Not turning them on unless necessary avoid temporary file creation etc. so I'm unsure. That… | |||||
| $this->timeout = $seconds; | $this->timeout = $seconds; | ||||
| return $this; | return $this; | ||||
| } | } | ||||
| /* -( Resolving Execution )------------------------------------------------ */ | /* -( Resolving Execution )------------------------------------------------ */ | ||||
| ▲ Show 20 Lines • Show All 73 Lines • ▼ Show 20 Lines | /* -( Resolving Execution )------------------------------------------------ */ | ||||
| public function resolveKill() { | public function resolveKill() { | ||||
| if (!$this->result) { | if (!$this->result) { | ||||
| if (defined('SIGKILL')) { | if (defined('SIGKILL')) { | ||||
| $signal = SIGKILL; | $signal = SIGKILL; | ||||
| } else { | } else { | ||||
| $signal = 9; | $signal = 9; | ||||
| } | } | ||||
| if (phutil_is_windows()) { | |||||
| // NOTE: See http://stackoverflow.com/a/28045959/90297 | |||||
| $status = proc_get_status($this->proc); | |||||
| exec('taskkill /F /T /PID '.$status['pid']); | |||||
| } else { | |||||
| proc_terminate($this->proc, $signal); | proc_terminate($this->proc, $signal); | ||||
| } | |||||
| $this->result = array( | $this->result = array( | ||||
Not Done Inline ActionsSame as my previous inline comment. This is a separate issue which I needed to fix to get tests running properly on Windows. BYK: Same as my previous inline comment. This is a separate issue which I needed to fix to get tests… | |||||
Not Done Inline ActionsBtw. context on this change: http://stackoverflow.com/a/28045959/90297 BYK: Btw. context on this change: http://stackoverflow.com/a/28045959/90297 | |||||
| 128 + $signal, | 128 + $signal, | ||||
| $this->stdout, | $this->stdout, | ||||
| $this->stderr, | $this->stderr, | ||||
| ); | ); | ||||
| $this->closeProcess(); | $this->closeProcess(); | ||||
| } | } | ||||
| return $this->result; | return $this->result; | ||||
| ▲ Show 20 Lines • Show All 151 Lines • ▼ Show 20 Lines | if (!$this->pipes) { | ||||
| $unmasked_command = $this->command; | $unmasked_command = $this->command; | ||||
| if ($unmasked_command instanceof PhutilCommandString) { | if ($unmasked_command instanceof PhutilCommandString) { | ||||
| $unmasked_command = $unmasked_command->getUnmaskedString(); | $unmasked_command = $unmasked_command->getUnmaskedString(); | ||||
| } | } | ||||
| $pipes = array(); | $pipes = array(); | ||||
| if (phutil_is_windows()) { | |||||
| // See T4395. proc_open under Windows uses "cmd /C [cmd]", which will | |||||
| // strip the first and last quote when there aren't exactly two quotes | |||||
| // (and some other conditions as well). This results in a command that | |||||
| // looks like `command" "path to my file" "something something` which is | |||||
| // clearly wrong. By surrounding the command string with quotes we can | |||||
| // be sure this process is harmless. | |||||
| if (strpos($unmasked_command, '"') !== false) { | |||||
| $unmasked_command = '"'.$unmasked_command.'"'; | |||||
| } | |||||
| } | |||||
| if ($this->hasEnv()) { | if ($this->hasEnv()) { | ||||
| $env = $this->getEnv(); | $env = $this->getEnv(); | ||||
| } else { | } else { | ||||
| $env = null; | $env = null; | ||||
| } | } | ||||
| $cwd = $this->getCWD(); | $cwd = $this->getCWD(); | ||||
| Show All 23 Lines | if (!$this->pipes) { | ||||
| } | } | ||||
| $proc = @proc_open( | $proc = @proc_open( | ||||
| $unmasked_command, | $unmasked_command, | ||||
| $spec, | $spec, | ||||
| $pipes, | $pipes, | ||||
| $cwd, | $cwd, | ||||
| $env); | $env); | ||||
Not Done Inline ActionsThis will break anything that doesn't specify an absolute path to the binary program, and will break any invocations that make use of shell features such as &&. This is not the correct fix. hach-que: This will break anything that doesn't specify an absolute path to the binary program, and will… | |||||
Not Done Inline ActionsIt actually didn't break things that are not specifying an absolute path (see my tests below using just php) that said you have a valid point with &&. I just don't want to rely on CMD.exe's horribly broken stuff but looks like the solution you want is to simply throw an exception when it sees a new line. Technically LF should work where as all CRs are removed automatically but I can't even get LF working for some reason. BYK: It actually didn't break things that are not specifying an absolute path (see my tests below… | |||||
| if ($this->useWindowsFileStreams) { | if ($this->useWindowsFileStreams) { | ||||
| fclose($spec[1]); | fclose($spec[1]); | ||||
| fclose($spec[2]); | fclose($spec[2]); | ||||
| $pipes = array( | $pipes = array( | ||||
| 0 => head($pipes), // stdin | 0 => head($pipes), // stdin | ||||
| 1 => fopen($this->windowsStdoutTempFile, 'rb'), // stdout | 1 => fopen($this->windowsStdoutTempFile, 'rb'), // stdout | ||||
| 2 => fopen($this->windowsStderrTempFile, 'rb'), // stderr | 2 => fopen($this->windowsStderrTempFile, 'rb'), // stderr | ||||
| ); | ); | ||||
| ▲ Show 20 Lines • Show All 102 Lines • ▼ Show 20 Lines | if (!$status['running']) { | ||||
| // consider the subprocess to have exited until we've read everything. | // consider the subprocess to have exited until we've read everything. | ||||
| // See T9724 for context. | // See T9724 for context. | ||||
| if (feof($stdout) && feof($stderr)) { | if (feof($stdout) && feof($stderr)) { | ||||
| $is_done = true; | $is_done = true; | ||||
| } | } | ||||
| } | } | ||||
| if ($is_done) { | if ($is_done) { | ||||
| if ($this->useWindowsFileStreams) { | |||||
| fclose($stdout); | |||||
| fclose($stderr); | |||||
| } | |||||
Not Done Inline ActionscloseProcess already closes all pipes so this is redundant. BYK: `closeProcess` already closes all pipes so this is redundant. | |||||
| $this->result = array( | $this->result = array( | ||||
| $status['exitcode'], | $status['exitcode'], | ||||
| $this->stdout, | $this->stdout, | ||||
| $this->stderr, | $this->stderr, | ||||
| ); | ); | ||||
| $this->closeProcess(); | $this->closeProcess(); | ||||
| return true; | return true; | ||||
| } | } | ||||
| ▲ Show 20 Lines • Show All 127 Lines • Show Last 20 Lines | |||||
This fix should probably be in its own patch but I needed it to get my tests passing.