Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Process and chained commands in symfony 2.2 #5759

Closed
romainneutron opened this Issue Oct 15, 2012 · 10 comments

Comments

Projects
None yet
5 participants
@romainneutron
Copy link
Member

romainneutron commented Oct 15, 2012

Hello,

I wanted to implement signal and getPid methods for 2.2 (see #5476).

  • Posix signal are very interesting in case of timeout ; when you ask your process to stop, you could specify a timeout when a SIGKILL could be sent. (And I'm sure you got lots of ideas to use them other ways)
  • Pid can be useful for interactions with other programs.

The Story :

There are two functions to implement these methods : proc_get_status and proc_terminate.
Actually, these methods might not work as expected inside the Process component because PHP might not run the command directly, but wraps it with sh (described in #5030).

Reminder

Chained commands

The Process component can handle such command :

new Process('git branch -a && git fetch upstream');

Inside Process, it can be summarized as :

$descriptorspec = array(array("pipe", "r"), array("pipe", "w"), array("pipe", "a"));
$process = proc_open('git branch -a && git fetch upstream', $descriptorspec, $pipes);

The sh wrapper

Let's run a command and let's ask for the Pid

$descriptorspec = array(array("pipe", "r"), array("pipe", "w"), array("pipe", "a"));
$process = proc_open('sleep 3', $descriptorspec, $pipes);
$status = proc_get_status($process);
echo $status['pid'];

will result in :

15888

That can be monitored :

grosroro 15886  1.0  0.0 254140 16164 pts/0    S+   23:19   0:00  | \_ php proc.php     
grosroro 15888  0.0  0.0   3940   580 pts/0    S+   23:19   0:00  |     \_ sh -c sleep 3
grosroro 15889  0.0  0.0   6748   568 pts/0    S+   23:19   0:00  |         \_ sleep 3

The actual Pid is 15889, not 15888 ; The status returned by proc_get_status is not the one we expected (the status of our command), but the one of the sh wrapper.
It is the same behavior with proc_terminate ; the signal is sent to the sh wrapper instead of the command.

The problem

This leads us to two conclusions :

  • All the methods that returns results or use results probed by proc_get_status might be wrong (hasBeenSignaled, getTermSignal, hasBeenStopped, getStopSignal, stop).
  • We can not currently easily signal a process or retrieve the pid in the current design.

The solution

The only solution found is to prepend the command with [exec](https://en.wikipedia.org/wiki/Exec_(operating_system\)) :

$descriptorspec = array(array("pipe", "r"), array("pipe", "w"), array("pipe", "a"));
$process = proc_open('exec sleep 3', $descriptorspec, $pipes);
$status = proc_get_status($process);
echo $status['pid'];

will result in :

9441

That can be monitored :

grosroro  9440  1.0  0.0 254140 16188 pts/0    S+   23:48   0:00  |   \_ php proc.php
grosroro  9441  0.0  0.0   6748   564 pts/0    S+   23:48   0:00  |       \_ sleep 3

proc_terminate and proc_get_status works as expected in this case.

Backward Compatibility

This solution is quite nice, but it leads to another issue :

This was working well :

$descriptorspec = array(array("pipe", "r"), array("pipe", "w"), array("pipe", "a"));
$process = proc_open('echo "hello" && echo "world"', $descriptorspec, $pipes);
// echoes "hello\nworld";
echo fgets($pipes[1], 4096);

This is not working as expected :

$descriptorspec = array(array("pipe", "r"), array("pipe", "w"), array("pipe", "a"));
$process = proc_open('exec echo "hello" && echo "world"', $descriptorspec, $pipes);
// echoes "hello";
echo fgets($pipes[1], 4096);

Proposals

Parse command line and allow pid/signal access to non-chained commands

this would prepend command line with "exec" only for atomic commands.

  • pros : preserve BC 2.1
  • cons : Lots of methods might not work as expected (hasBeenSignaled, getTermSignal, hasBeenStopped, getStopSignal, stop)

Split commands in atomic subprocesses / add a chain method / enforce use of ProcessBuilder

  • pros : guarantee the control control over the processes, adds multiple possibilities for chaining strategies), fixes
  • cons : breaks the BC with 2.1, disable the ability to run chained commands in the background (There is no way to add a callback when a process finish to start the next one)

Do you have any thoughts about this ?

@stof

This comment has been minimized.

Copy link
Member

stof commented Oct 16, 2012

@romainneutron I don't see why you say that getExitCode does not return the right result. It does, according to the same rule than used by sh. ; and && are not equivalent (otherwise, we would not have 2 syntaxes)

@romainneutron

This comment has been minimized.

Copy link
Member Author

romainneutron commented Oct 16, 2012

You are correct.

@romainneutron

This comment has been minimized.

Copy link
Member Author

romainneutron commented Oct 16, 2012

I removed this block :

Chained command interpretation

In the following example we have a second problem due to chained commands. Both commands are legals with the Process component but not interpreted the same way by a shell. In bash, the exit code is 127 for $process1 and 0 for $process2.

$process1 = new Process('nonExistingCommand && ls');
$process1->run();

$process2 = new Process('nonExistingCommand ; ls');
$process2->run();

assert($process1->getExitCode() !== $process2->getExitCode());
@Seldaek

This comment has been minimized.

Copy link
Member

Seldaek commented Oct 16, 2012

How about a ProcessChain class that would take a list of Process/ProcessChain instances and run them one by one, it could run in AND or OR mode - so in AND mode it would stop as soon as something exits with non-zero. It would be stoppable at any time and return the current Pid of whichever subprocess it's currently running, all other calls could be forwarded to the current subprocess as well.

Nesting of ProcessChain allows you to build AND/OR combinations if really needed.

@romainneutron

This comment has been minimized.

Copy link
Member Author

romainneutron commented Oct 16, 2012

Quick summary of IRC conversation :

@schmittjoh propose to parse commands and run them atomically with a regexp like preg_split('#("(?:[^"]|"(?<=\\\\))*"|'(?:[^']|'(?<=\\\\))*'|[^\s]+)#', $cmd, null, PREG_SPLIT_*)

@Seldaek said that with such commands for i in ls foo; do echo $i && cat $i; done there would be less fun, propose to reduce magic that could leads us to weird bugs

Seldaek: the ProcessChain isn't soo complex and it could also just accept an array of commands that it turns into Process instances itself

With jordi proposal, BC is preserved

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Oct 27, 2012

I agree with @Seldaek

@jmikola

This comment has been minimized.

Copy link
Contributor

jmikola commented May 28, 2013

@romainneutron: Are there implications here for the command wrapping already done for Sigchild compatibility?

See: https://github.com/symfony/Process/blob/master/Process.php#L1117

if ($this->enhanceSigchildCompatibility && $this->isSigchildEnabled()) {
    // last exit code is output on the fourth pipe and caught to work around --enable-sigchild
    $descriptors = array_merge($descriptors, array(array('pipe', 'w')));

    $this->commandline = '('.$this->commandline.') 3>/dev/null; code=$?; echo $code >&3; exit $code';
}
@romainneutron

This comment has been minimized.

Copy link
Member Author

romainneutron commented May 28, 2013

This commandline is a hack to get the exitcode of a process in a sigchild environment (otherwise it returns -1).
Is this the answer you were looking for ?

@jmikola

This comment has been minimized.

Copy link
Contributor

jmikola commented May 28, 2013

No, I was curious if using exec to effectively replace the sh process (instead of having our process launched as a child of sh) would still work if we wrap $this->commandline in parentheses.

@romainneutron

This comment has been minimized.

Copy link
Member Author

romainneutron commented May 28, 2013

In the case of sigchild environment, it won't work. Have a look at the output of :

use Symfony\Component\Process\Process;

$process = new Process('echo "a"; echo "b"');
$process->run();
var_dump($process->getOutput());

$process = new Process('exec (echo "a"; echo "b")');
$process->run();
var_dump($process->getOutput());

$process = new Process('exec echo "a"; echo "b"');
$process->run();
var_dump($process->getOutput());
string(4) "a\nb\n"
string(0) ""
string(2) "a\n"

@fabpot fabpot closed this Mar 13, 2014

xabbuh added a commit to xabbuh/symfony that referenced this issue Jul 5, 2014

only escape command arguments and not the command itself
Otherwise, prefixing a command with exec (see symfony#5759 why you would do that) would lead to a "Command not found error".

xabbuh added a commit to xabbuh/symfony that referenced this issue Aug 28, 2014

ProcessBuilder: add method to use the sh wrapper
On Linux, processes built and executed with the Process component are
automatically wrapped by PHP inside a call to `sh`. This makes it
hard to use methods like `getPid()` or `stop()` since they work with
the `sh` process but not with the process the user intended to start.
The solution is to prepend the command being executed with
[exec](https://en.wikipedia.org/wiki/Exec_%28operating_system%29) (as
described in symfony#5759). The `ProcessBuilder` now has two methods
`enableShellWrapper()` and `disableShellWrapper()` to enable or
disable the shell wrapper.

xabbuh added a commit to xabbuh/symfony that referenced this issue Aug 28, 2014

add method to ProcessBuilder to use the sh wrapper
On Linux, processes built and executed with the Process component are
automatically wrapped by PHP inside a call to `sh`. This makes it
hard to use methods like `getPid()` or `stop()` since they work with
the `sh` process but not with the process the user intended to start.
The solution is to prepend the command being executed with
[exec](https://en.wikipedia.org/wiki/Exec_%28operating_system%29) (as
described in symfony#5759). The `ProcessBuilder` now has two methods
`enableShellWrapper()` and `disableShellWrapper()` to enable or
disable the shell wrapper.

bogdanghervan added a commit to cronkeep/cronkeep that referenced this issue Nov 8, 2014

- Using "at" command to start processes in the background, outside th…
…e web server's reach.

- Showing instructions and debug hints if "at" is not available or Apache is denied access to the command.
- Added autocomplete="off" to button elements so they don't retain state after the page is refreshed in Firefox.
- No longer showing the PID in success message shown after a job is run (since it's not very accurate as described here symfony/symfony#5759).

edhelas added a commit to movim/movim that referenced this issue Jul 19, 2015

edhelas added a commit to movim/movim that referenced this issue Jul 21, 2015

fabpot added a commit that referenced this issue Feb 8, 2017

feature #21474 [Process] Accept command line arrays and per-run env v…
…ars, fixing signaling and escaping (nicolas-grekas)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[Process] Accept command line arrays and per-run env vars, fixing signaling and escaping

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #12488, #11972, #10025, #11335, #5759, #5030, #19993, #10486
| License       | MIT
| Doc PR        | -

I think I found a way to fix this network of issues once for all.
Of all the linked ones, only the last two are still open: the remaining were closed in dead ends.

Instead of trying to make `ProcessUtil::escapeArgument` work correctly on Windows - which is impossible as discussed in #21347 - this PR deprecates it in favor of a more powerful approach.

Depending on the use case:

- when a simple command should be run, `Process` now accepts an array of arguments (the "binary" being the first arg). Making this the responsibility of `Process` (instead of `ProcessBuilder`) gives two benefits:
  - escape becomes an internal detail that doesn't leak - thus can't be misused ([see here](#21347 (comment)))
  - since we know we're running a single command, we can prefix it automatically by "exec" - thus fixing a long standing issue with signaling

```php
        $p = new Process(array('php', '-r', 'echo 123;'));
        echo $p->getCommandLine();
        // displays on Linux:
        // exec 'php' '-r' 'echo 123;'
```

- when a shell expression is required, passing a string is still allowed. To make it easy and look-like sql prepared statements, env vars can be used when running the command. Since the shell is OS-specific (think Windows vs Linux) - this PR assumes no portability, so one should just use each shell's specific syntax.

From the fixtures:
```php
        $env = array('FOO' => 'Foo', 'BAR' => 'Bar');
        $cmd = '\\' === DIRECTORY_SEPARATOR ? 'echo !FOO! !BAR! !BAZ!' : 'echo $FOO $BAR $BAZ';
        $p = new Process($cmd, null, $env);
        $p->run(null, array('BAR' => 'baR', 'BAZ' => 'baZ'));

        $this->assertSame('Foo baR baZ', rtrim($p->getOutput()));
        $this->assertSame($env, $p->getEnv());
```

Commits
-------

330b61f [Process] Accept command line arrays and per-run env vars, fixing signaling and escaping

aschempp added a commit to terminal42/background-process that referenced this issue May 11, 2017

glatzor added a commit to glatzor/dusk that referenced this issue Sep 3, 2017

Use Process instead of ProcessBuilder to stop chromedriver afterwards
For backwards compatibilty reasons simfony's ProcessBuilder doesn't prepend
the exec command to the commandline. So only the PID of the wrapping
shell gets stored, but not the one of the chromedriver.

A solution is to use Process directly.

See symfony/symfony#5759 for background
information.

Closes laravel#341
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.