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

Passing SIGINT to script process #6059

Closed
AdrianSkierniewski opened this issue Jan 11, 2017 · 10 comments · Fixed by #10958
Closed

Passing SIGINT to script process #6059

AdrianSkierniewski opened this issue Jan 11, 2017 · 10 comments · Fixed by #10958
Labels
Milestone

Comments

@AdrianSkierniewski
Copy link

AdrianSkierniewski commented Jan 11, 2017

Hi,

I noticed that when I kill composer script command (like this below) using ctrl+c, Codeception is still running tests. The problem was more complex than and though at the beginning. Since PHP 7.0, ticks won't work as before (Codeception issue), but PHP 7.1 introduced new feature called pcntl_async_signals, so I decided to create PR for Codeception with fix for 7.1.
After fixing Codeception issue I noticed that composer isn't passing the signal to the child process. Later after some investigation, I manage to fix that by adding some PHP 7.1 code in ProcessExecutor (check above).

 "scripts": {
        ...
        "test": [
            "php vendor/bin/codecept run"
        ]
        pcntl_async_signals(true);
        pcntl_signal(SIGINT, function($sig) use ($process) {
            $process->stop(0, SIGINT);
        });

Unfortunately, my problem is even more complex, because in the middle I have a docker container and I can't force composer to run docker in TTY mode, so my signals won't reach Codeception after all. I'd probably create a custom script to run those tests instead putting them in composer scripts section, but I wanted to share my research with you guys.

 "scripts": {
        ...
        "test": [
            "docker exec -it project_web_server_1 php /var/www/vendor/bin/codecept run -c /var/www"

        ]
the input device is not a TTY
@alcohol
Copy link
Member

alcohol commented Jan 11, 2017

This is why I run composer through tini in Docker:

https://github.com/composer/docker/blob/master/docker-entrypoint.sh#L49-L60

@AdrianSkierniewski
Copy link
Author

@alcohol Thank you, I didn't know tini

@alcohol
Copy link
Member

alcohol commented Jan 12, 2017

I don't think it will help solve your use-case on second thought though. But it does help with some other issues you have or might be facing.

@AdrianSkierniewski
Copy link
Author

AdrianSkierniewski commented Jan 12, 2017 via email

@Seldaek Seldaek added this to the 1.4 milestone Jan 22, 2017
@dzuelke
Copy link
Contributor

dzuelke commented Jan 25, 2017

Part of the problem here is that CodeCeption does not exit correctly when it receives the signal.

Any program that exits as a result of a signal (say SIGINT or SIGTERM) must kill itself using that signal. That allows the calling program (a shell, or e.g. Composer) to determine the reason for the exit.

So for instance, to react to a Ctrl+C (which is sent to all processes in the group, something that further complicates the whole matter), restore the default handler in the callback, then kill yourself using the same signal:

pcntl_async_signals(true); // turn on async signals

pcntl_signal(SIGINT, function($sig, $info) {
	echo "ool, goodbye :)\n";
	pcntl_signal(SIGINT, SIG_DFL);
	posix_kill(posix_getpid(), SIGINT);
});

while(true) {
	echo "ping\n";
	sleep(1);
}

If you now run php test.php, Ctrl+C out of it, and then echo $?, you'll see "130" as the result. That's 128+$SIGNALCODE, and SIGINIT is "2".

Note that you have to do it this way. You cannot simply exit with status 130. The shell just prints it that way (128 plus signal code).

Also see https://www.cons.org/cracauer/sigint.html

Either way, Composer could then inspect the exit status of the child script, and draw its own conclusions. For example, if the child process exited on SIGINT, it would itself also shut down quickly. Otherwise, it would continue normally, as the child process ignored the signal (and it's the innermost child after all which gets do decide what happens; see again the link above). Contrived example: a script runs "emacs", where you Ctrl+C a lot ;)

Also, please note that killing "0" is not the same as posix_getpid(). The former kills the whole process group (well, on most systems at least), while the latter only kills the specific process. You definitely want to do the latter, to give the immediate child process a chance to do its own cleanup.

I'd also have to do some more digging around why the child process doesn't receive the signal. The shell should send it to the whole group already, without us having to forward it.

@dzuelke
Copy link
Contributor

dzuelke commented Jan 25, 2017

Also, Docker has its own unique set of issues around Ctrl+C, IIRC also related to its not-really-TTY nature.

@AdrianSkierniewski
Copy link
Author

@dzuelke Thank you for such a good explanation of this process. I already decided to remove composer from this equation to save me some of those docker related issues. I'd definitely like to see better support for SIGINT in various PHP libraries (when running as composer script or inside docker).

@dzuelke
Copy link
Contributor

dzuelke commented Jan 26, 2017

To clarify this further: the sub-process already receives the SIGINT, you just don't see anything it does anymore on shutdown because Composer exits immediately.

To test:

<?php

pcntl_async_signals(true); // turn on async signals

pcntl_signal(SIGINT, function($sig, $info) {
	echo "ool, goodbye :)\n";
	file_put_contents("erp", "erp");
	pcntl_signal(SIGINT, SIG_DFL);
	posix_kill(posix_getpid(), SIGINT);
});

while(true) {
	echo "ping\n";
	sleep(1);
}

?>
{
	"scripts": {
		"test": "php test.php"
	}
}

Then run composer test, and Ctrl+C out. You'll have a file named erp locally.

So Composer needs adjusting in such a way that it evaluates the child exit status, and on Ctrl+C simply waits for the child to exit. Maybe I'll do a PR, but it might be something that needs changing in the Symfony Process component.

In your particular case, it's more likely a Docker-induced interaction that's causing the problem.

@stof
Copy link
Contributor

stof commented Jan 26, 2017

Maybe I'll do a PR, but it might be something that needs changing in the Symfony Process component.

I don't think it is. The Process component does not deal with killing the main PHP process.

The issue is probably related to the fact that Composer does not do anything with signals AFAIK, and so the PHP process is killed as soon as PHP does it by itself

@Seldaek Seldaek modified the milestones: 1.5, 1.4 Mar 7, 2017
@Seldaek Seldaek modified the milestones: 1.5, 1.6 Aug 6, 2017
@Seldaek Seldaek modified the milestones: 1.6, Nice To Have Dec 18, 2017
@Seldaek Seldaek modified the milestones: Nice To Have, 2.4 May 24, 2022
@Seldaek
Copy link
Member

Seldaek commented Jul 20, 2022

Finally took a look at this ;) #10958 - happy to get a review @dzuelke if you have time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants