fix signal handling in wait() on calls to stop() #11436

xabbuh · 2014-07-21T20:15:43Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#11286
License	MIT
Doc PR

wait() throws an exception when the process was terminated by a signal. This should not happen when the termination was requested by calling the stop() method (for example, inside a callback which is passed to wait()).

fabpot · 2014-07-22T14:00:25Z

ping @romainneutron

romainneutron · 2014-07-22T14:04:36Z

The principle is right. However, I think we should store the latest signal value that has been sent and compare it with the termsig value to avoid the exception.

romainneutron · 2014-07-22T14:05:34Z

src/Symfony/Component/Process/Process.php

@@ -321,7 +324,7 @@ public function wait($callback = null)
            usleep(1000);
        }

-        if ($this->processInformation['signaled']) {
+        if ($this->processInformation['signaled'] && true !== $this->stopCalled) {


This would produce

if ($this->processInformation['signaled'] && $this->processInformation['termsig'] === $this->lastestSignal) {

Does this work? From what I see, with this change the exception wouldn't be thrown anymore even when the process was terminated with the UNIX kill command.

The purpose of the bug fix is to avoid sending an exception when the underlying process is signaled by the process component.
When we call the Process::stop method, we just send a signal to the underlying process.

kill is excatly the same as our Process::signal method : it sends a signal to a process.

Using the way I propose, any signal sent outside the Process component (using kill for example) would trigger the exception, not the ones sent by the Process component

Hope I'm clear enough :)

So, if I understand you correctly, the latest signal has to be modified inside stop() and inside signal() to be handled correctly?

@romainneutron I added an additional commit. If that's what you had in mind, I'll drop the first commit.

romainneutron · 2014-07-23T06:10:05Z

@xabbuh this approach is much better. However, we should update the way Process::stop works:

At line 664, the underlying process is signaled (proc_terminate does exactly the same as unix kill or Process::signal):

proc_terminate($this->process);

Instead of that, we should signal it with our custom method and the appropriate signal. proc_terminate use SIGTERM (15) as default value, so it should be replaced with

$this->doSignal(15);

Then, changes introduced in Process::stop at line 679 and 680 should be removed

xabbuh · 2014-07-24T19:02:32Z

@romainneutron I updated it accordingly. However, I was not quite sure if it is better to use 15 or the SIGTERM constant. I chose the hardcoded number since the PCNTL extension, from which the constant is coming from, might not be available. But on the other hand, there are other uses of the constant too.

Not sure why two timeout related tests are failing now. I will have a deeper look at them.

romainneutron · 2014-07-25T07:06:38Z

This is related to the sigchild environment. Before your change we were not detecting sigchild environment before signaling the underlying process with proc_terminate, it's now done in the Process::doSignal method

I'll have a look on this today

romainneutron · 2014-07-25T07:47:54Z

I've fixed unit tests in romainneutron@ec2f0ed You can cherry-pick my commit in your branch.

xabbuh · 2014-07-25T08:31:21Z

@romainneutron Thank you very much for your help! I added your commit and now all tests succeed.

romainneutron · 2014-07-25T08:33:49Z

src/Symfony/Component/Process/Process.php

@@ -661,7 +663,7 @@ public function stop($timeout = 10, $signal = null)
                    throw new RuntimeException('Unable to kill the process');
                }
            }


Can you add a comment regarding the line below?

// given `SIGTERM` may not be defined and that `proc_terminate` uses the constant value and not the constant itself, we use the same here

Of course, it's done now. Do you think we should replace the usage of those constants in some of the other lines where they are used?

no, not needed

romainneutron · 2014-07-25T08:34:06Z

Once the comment has been added, 👍

wait() throws an exception when the process was terminated by a signal. This should not happen when the termination was requested by calling either the stop() or the signal() method (for example, inside a callback which is passed to wait()).

fabpot · 2014-07-25T09:23:14Z

👍

romainneutron · 2014-07-25T09:24:04Z

Thank you @xabbuh.

…romainneutron) This PR was merged into the 2.3 branch. Discussion ---------- fix signal handling in wait() on calls to stop() | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #11286 | License | MIT | Doc PR | ``wait()`` throws an exception when the process was terminated by a signal. This should not happen when the termination was requested by calling the ``stop()`` method (for example, inside a callback which is passed to ``wait()``). Commits ------- 5939d34 [Process] Fix unit tests in sigchild environment eb68662 [Process] fix signal handling in wait() 94ffc4f bug #11469 [BrowserKit] Fixed server HTTP_HOST port uri conversion (bcremer, fabpot) 103fd88 [BrowserKit] refactor code and fix unquoted regex f401ab9 Fixed server HTTP_HOST port uri conversion 045cbc5 bug #11425 Fix issue described in #11421 (Ben, ben-rosio) f5bfa9b bug #11423 Pass a Scope instance instead of a scope name when cloning a container in the GrahpvizDumper (jakzal) 3177be5 minor #11464 [Translator] Use quote to surround invalid locale (lyrixx) c9742ef [Translator] Use quote to surround invalid locale 4dbe0e1 bug #11120 [2.3][Process] Reduce I/O load on Windows platform (romainneutron) 797d814 bug #11342 [2.3][Form] Check if IntlDateFormatter constructor returned a valid object before using it (romainneutron) 0b5348e minor #11441 [Translator] Optimize assertLocale regexp (Jérémy Derussé) 537c39b Optimize assertLocale regexp 4cf50e8 Bring code into standard 9f4313c [Process] Add test to verify fix for issue #11421 02eb765 [Process] Fixes issue #11421 6787669 [DependencyInjection] Pass a Scope instance instead of a scope name. 9572918 bug #11411 [Validator] Backported #11410 to 2.3: Object initializers are called only once per object (webmozart) 291cbf9 [Validator] Backported #11410 to 2.3: Object initializers are called only once per object efab884 bug #11403 [Translator][FrameworkBundle] Added @ to the list of allowed chars in Translator (takeit) 3176f8b [Translator][FrameworkBundle] Added @ to the list of allowed chars in Translator 91e32f8 bug #11381 [2.3] [Process] Use correct test for empty string in UnixPipes (whs, romainneutron) 45df2f3 minor #11397 [2.3][Process] Fix unit tests on Windows platform (romainneutron) cec0a45 [Process] Adjust PR #11264, make it Windows compatible and fix CS d418935 [Process] Fix unit tests on Windows platform ff0bb01 [Process] Reduce I/O load on Windows platform ace5a29 bumped Symfony version to 2.3.19 75e07e6 updated VERSION for 2.3.18 4a12f4d update CONTRIBUTORS for 2.3.18 98b891d updated CHANGELOG for 2.3.18 06a80fb Validate locales sets intos translator 06fc97e feature #11367 [HttpFoundation] Fix to prevent magic bytes injection in JSONP responses... (CVE-2014-4671) (Andrew Moore) 3c54659 minor #11387 [2.3] [Validator] Fix UserPassword validator translation (redstar504) 73d50ed Fix UserPassword validator translation 93a970c bug #11386 Remove Spaceless Blocks from Twig Form Templates (chrisguitarguy) 8f9ed3e Remove Spaceless Blocks from Twig Form Templates 9e1ea4a [Process] Use correct test for empty string in UnixPipes 6af3d05 [HttpFoundation] Fix to prevent magic bytes injection in JSONP responses (Prevents CVE-2014-4671) ebf967d [Form] Check if IntlDateFormatter constructor returned a valid object before using it

xabbuh mentioned this pull request Jul 21, 2014

[Process] Getting a "Process has been signaled" exception when calling $process->stop() #11286

Closed

romainneutron added the Process label Jul 21, 2014

romainneutron reviewed Jul 22, 2014
View reviewed changes

romainneutron reviewed Jul 25, 2014
View reviewed changes

xabbuh and others added 2 commits July 25, 2014 10:39

[Process] fix signal handling in wait()

eb68662

wait() throws an exception when the process was terminated by a signal. This should not happen when the termination was requested by calling either the stop() or the signal() method (for example, inside a callback which is passed to wait()).

[Process] Fix unit tests in sigchild environment

5939d34

romainneutron merged commit 5939d34 into symfony:2.3 Jul 25, 2014

xabbuh deleted the issue-11286 branch July 25, 2014 09:25

jakzal mentioned this pull request Aug 1, 2014

[Process] Difference between Mac and Linux #11543

Closed

Uh oh!

fix signal handling in wait() on calls to stop() #11436

fix signal handling in wait() on calls to stop() #11436

Uh oh!

Conversation

xabbuh commented Jul 21, 2014

Uh oh!

fabpot commented Jul 22, 2014

Uh oh!

romainneutron commented Jul 22, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romainneutron commented Jul 23, 2014

Uh oh!

xabbuh commented Jul 24, 2014

Uh oh!

romainneutron commented Jul 25, 2014

Uh oh!

romainneutron commented Jul 25, 2014

Uh oh!

xabbuh commented Jul 25, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romainneutron commented Jul 25, 2014

Uh oh!

fabpot commented Jul 25, 2014

Uh oh!

romainneutron commented Jul 25, 2014

Uh oh!

Uh oh!