8000 [Process] Stopping of processes inconsistent between platforms · Issue #20259 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Process] Stopping of processes inconsistent between platforms #20259

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.

Sign up for GitHub

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

Jump to bottom
Closed
sustmi opened this issue Oct 20, 2016 · 8 comments
Closed

[Process] Stopping of processes inconsistent between platforms #20259

sustmi opened this issue Oct 20, 2016 · 8 comments
Labels
Process Status: Needs Review

Comments

@sustmi
Copy link
Contributor
sustmi commented Oct 20, 2016 •
edited
Loading

On Linux, the primary implementation of Process::stop() is calling proc_terminate() with TERM signal.
This effectively kills the process but usually not its child processes.

On Windows, however, taskkill command with /T switch is used so whole process sub-tree is killed.

I found that the taskkill command for Windows platforms was added here: symfony/process@1eb5593#diff-9a01fc0e340da4c3f1e4a16029a63977R626 .
But the commit message says nothing about any need to kill the whole process tree:

[Process] Make Process::start non-blocking on Windows platform

Is there any reason for this inconsistency?
@sustmi
Copy link
Contributor Author
sustmi commented Oct 20, 2016 •
edited
Loading

I would expect Process::stop() to kill the whole process tree on all platforms (even when POSIX kill() does not do it I would expect the Process component to clean-up everything).
This is a common behaviour in Linux terminal (eg. bash) that when you press CTRL+C to stop the current process all the child processes are stopped too.

@sustmi sustmi changed the title [Process] Inconsistent stopping of processes between platforms [Process] Stopping of processes inconsistent between platforms Oct 20, 2016
@xabbuh xabbuh added the Process label Oct 21, 2016
@nicolas-grekas
Copy link
Member

Linux and Windows are two different OSes and we can't completely abstract them out.
PHP wraps all process in a shell by default. On Linux, you can replace this shell by your actual command by prefixing it with exec (eg. exec command --arg instead of command --arg).
This is not something we can fix in Symfony actually and unfortunately.

@javiereguiluz javiereguiluz removed the Bug label Jul 17, 2017
@javiereguiluz
Copy link
Member

@sustmi I'm sorry but I must close this as "we can't fix it" because of the reasons mentioned by Nicolas.

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Jul 17, 2017 •
edited
Loading

Not that this has been "fixed" in 3.3, when you use the Process class with an array as first attribute.

@sustmi sustmi mentioned this issue Jul 22, 2017
Closed
@sustmi
Copy link
Contributor Author
sustmi commented Jul 22, 2017 •
edited
Loading

If I remember it correctly I wanted the Process::stop() call to kill the child processes to solve this issue in Composer: composer/composer#5800 .
In brief, when Composer thinks that git clone takes too much time it calls Process::stop() but this stops only the parent process and not the child ones. Composer then downloads the files by some fallback method. But in the meantime Git's child process finishes the download however as it detects that the target folder was changed (by Composer doing the fallback download method) it "panics" and deletes all the files.

I do not think that this issue can be resolved by passing an array as an argument to Process constructor.
Luckily I did not encounter the problem again since 2016 so lets hope it will not appear again.

@nicolas-grekas
Copy link
Member

I do not think that this issue can be resolved by passing an array as an argument to Process constructor.

That's a strange statement... How do you know?
Personally, I know that I worked on a PR that's been merged into 3.3, that when you pass an array as first argument to Process, is able to remove the intermediary shell, so that git is not a child process anymore, and as such receives itself the kill signal...

@sustmi
Copy link
Contributor Author
sustmi commented Jul 22, 2017 •
edited 8DF2
Loading

If I understand it correctly the new "array as the first argument" interface (#21474) can only be used for a single executable.
The problem is that in Composer they do not use a simple command with a single executable but they want to chain commands using &&. See: https://github.com/composer/composer/blob/master/src/Composer/Downloader/GitDownloader.php#L65.
Or do I miss some point?

@nicolas-grekas
Copy link
Member

On Symfony side, there is a new primitive that allows fixing the issue. The composer would need to take advantage of it. The line you linked can certainly be split into several consecutive process calls if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Process Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@javiereguiluz @nicolas-grekas @sustmi @xabbuh @carsonbot
0