[Process] Add signal and getPid methods #5476

romainneutron · 2012-09-09T21:48:02Z

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
License of the code: MIT

This PR replaces #5391 ; it adds :

sigchild compatibility mode. This means that if you activate it you have access to this exitcode of the process in case your php has been compiled with --enable-sigchild. This can happen when you deal with Oracle databases.

$process->setEnhanceSigchildCompatibility(true);

getPid method to get the actual process identifier of the process

$process->getPid();

signal method to send Posix signal to the process

$process->signal(SIGHUP);

Add optionnal $signal as second argument to stopmethod. If provided, the signal is sent at timeout to the process. Example of use :

$process->stop(5, SIGKILL);

Tests have been enhanced and now run both sigchild / non-sigchild mode.

By the way, tests are successful with a PHP compiled with the --enable-sigchild option ;)

stof · 2012-09-09T22:06:12Z

src/Symfony/Component/Process/Tests/AbstractProcessTest.php

+
+    public function testProcessWithoutTermSignal()
+    {
+        if (strpos(PHP_OS, "WIN") === 0) {


you should use defined('PHP_WINDOWS_VERSION_BUILD') to detect windows

stof · 2012-09-18T15:52:55Z

I think the flag about the sigchild compatibility mode should be extracted and merged into 2.1 as the Process::stop method is broken currently because of the sigchild layer, so it should be activated only when it is needed.
@fabpot what do you think about it ?

romainneutron · 2012-09-18T16:05:04Z

I can easily cherry pick the commit and PR to 2.1

Romain

On 18 sept. 2012, at 17:53, Christophe Coevoet notifications@github.com
wrote:

I think the flag about the sigchild compatibility mode should be extracted
and merged into 2.1 as the Process::stop method is broken currently because
of the sigchild layer, so it should be activated only when it is needed.
@fabpot https://github.com/fabpot what do you think about it ?

—
Reply to this email directly or view it on
GitHubhttps://github.com//pull/5476#issuecomment-8659603.

Tobion · 2012-09-18T16:05:12Z

src/Symfony/Component/Process/Process.php

+        return $this->enhanceSigchildCompatibility;
+    }
+
+    public function setEnhanceSigchildCompatibility($enhance)


missing phpdoc

romainneutron · 2012-09-18T16:51:31Z

Wow :) thought my PR was already stoff'ed

except the typo, and the static declaration - that IMHO is more logic here as it would never be used outside the function scope - I've just imitated what already was in the component, moreover in symfony 2

What do you, @stof, think about it ?

marijn · 2012-09-18T16:53:40Z

Nice work. This is should really be merged back into 2.1 as well though. The stop method is simply broken for long running background processes without this fix.

stof · 2012-09-18T19:04:11Z

@marijn As I said above, only part of it should go in 2.1 (the bugfix). The new features can only go in 2.2

marijn · 2012-09-18T19:20:16Z

@stof that's great! I was just voicing my content with this PR 😄

romainneutron · 2012-09-18T19:43:22Z

@Tobion I changed what you suggested, except the isser for getEnhanceSigchildCompatibility to be consistent with the windows compatibility

pborreli · 2012-09-18T20:01:30Z

src/Symfony/Component/Process/Process.php

+    }
+
+    /**
+     * Activate sigchild comptaibility mode


comptaibility => compatibility

romainneutron · 2012-09-18T20:08:30Z

Okay, I fixed everything.
BTW I fixed the different typo / code / doc in their corresponding commits, so it's perfectly ok to cherry pick any of these

stof · 2012-09-18T20:14:02Z

src/Symfony/Component/Process/Process.php

@@ -51,6 +55,8 @@ class Process
    private $fileHandles;
    private $readBytes;

+    protected static $sigchild;


the property should be private (we use private by default and switch stuff to protected when we have a valid use case for it, as protected stuff are potential extension point and so are concerned by the BC)

stof · 2012-09-18T20:16:01Z

@romainneutron please send a PR based on 2.1 with the sigchild stuff as it is needed to fix the stop method for long running processes (so that it only break when using sigchild, not for everyone).

pborreli · 2012-09-18T20:50:26Z

src/Symfony/Component/Process/Process.php

+        ob_start();
+        phpinfo(INFO_GENERAL);
+
+        return self::$sigchild = false !== strpos(ob_get_clean(), '--enable-sigchild');


ubuntu php package don't show configure command options in phpinfo, dunno if sigchild is enabled in repo package but you can't know it with this method.

It's the only way to know it.
BTW, it seems this option is only used by OCI8 users ; ubuntu does not provide PHP with this option which seems quite exotic to distro packages... maybe RedHat ?

I don't think any distro ships with this on, it's a legacy switch that
some people compiling php manually still have for historical reasons
more than concrete reasons IMO (except for the few oracle+php users).

@schmittjoh

Commits ------- 7bafc69 Add a Sigchild compatibility mode (set to false by default) Discussion ---------- [2.1][Process] Fix stop in non-sigchild environments Bug fix: yes Feature addition: yes Backwards compatibility break: no Symfony2 tests pass: yes License of the code: MIT Fix #5030 in half way. - `proc_terminate` now sends the `SIGTERM` to the real process, not the sh (add the exec prefix to remove the wrapper as suggested by @schmittjoh). So now, process will stop, except if you're working with a PHP compiled with `--enable-sigchild` - Add a Sigchild compatibility mode (activated only if PHP has been compiled with it) This mode is required to use a hack to determine if the process finished with success when PHP has been compiled with the --enable-sigchild option #5030 will be totally fixed in 2.2 with #5476 as it would allow to send a `SIGKILL` after timeout --------------------------------------------------------------------------- by stof at 2012-09-18T21:19:50Z This will also fix the error reported in Behat/MinkZombieDriver#10 The stop method was broken because of the sigchild workaround introduced in the latest 2.1-RC times

fabpot · 2012-10-04T14:28:26Z

@romainneutron Can you rebase and squash this PR? Thanks.

romainneutron · 2012-10-04T14:39:18Z

Hello @fabpot ,

I can squash and rebase but this PR would not work as we removed the exec prefix (see #5030) and this is required to signal and retrieve pid.

I'm about to ask the community what we should do about the process component. We have two option : break BC and add chainable command support or add another class like @schmittjoh proposed.

I would prefer to break BC in 2.2 and re-think the whole thing as going further with the current design will lead to "different classes for each use" which is not very user/developer friendly I think.

I currently do not have so much time to make propositions, but I'll have some more time this week-end or next week.

Are you ok with that ?

fabpot · 2012-10-04T14:44:12Z

If we can fix design problems at the cost of slightly breaking BC, that's fine.

stof · 2012-10-04T15:35:54Z

@fabpot The BC break done previously was "stop supporting the chaining of commands". So it was a big BC break (breaking composer for instance). so we would need to find a way to continue supporting them before re-adding exec

fabpot · 2012-10-27T13:41:58Z

Closing in favor of #5759

fabpot · 2013-04-09T07:57:16Z

After thinking about this a bit more, I think we need to merge this PR without the exec trick. Instead, we document the known limitations and the fact that using exec is a nice workaround for 90% of the cases.

Implementing #5759 can then be done later without blocking the features of this PR that are useful when you know what your are doing.

What do you think? ping @romainneutron @schmittjoh @Seldaek

romainneutron · 2013-04-09T08:06:38Z

👍

romainneutron · 2013-04-09T14:51:06Z

If everybody's okay, I'll give some love to this PR tonight.

@fabpot I'll also enhance the documentation to give information about the limitations. Should I do it in the Process component doc or should I rather create an entry in the cookbook ?

romainneutron · 2013-04-09T23:43:40Z

The PR has been updated according latest proposal.
An update has been provided to the Process::stop method which now has a $signal as second argument. The default value for this optional signal is null because I don't think that the SIGKILL constant is defined on windows. (previous behavior was to send a SIGKILL signal as the platform is not Windows).

I'm open to your suggestions.

Pending :

Documentation

fabpot · 2013-04-10T06:21:35Z

src/Symfony/Component/Process/Process.php

+    public function getPid()
+    {
+        if ($this->isSigchildEnabled()) {
+            throw new RuntimeException(


This should be on one line only (same below).

it is a single line. Github is wrapping the lines when they are too long now (see the line numbers and you will see it is the same line)

fabpot · 2013-04-10T06:23:28Z

Looks good to me. Documentation should probably go into the main Process documentation chapter.

romainneutron · 2013-04-14T16:21:23Z

Ping @fabpot I've added some docs, let me know if anything is missing on this one.

romainneutron · 2013-04-14T16:24:54Z

Updated PR info : removed fixing bug #5030 as it's not true anymore

romainneutron mentioned this pull request Sep 9, 2012

[2.2] [Process] Add getPid and sendSignal methods to Process #5391

Closed

stof reviewed Sep 9, 2012
View reviewed changes

Tobion reviewed Sep 18, 2012
View reviewed changes

pborreli reviewed Sep 18, 2012
View reviewed changes

src/Symfony/Component/Process/Process.php

}

/**

* Activate sigchild comptaibility mode

Copy link

Contributor

pborreli Sep 18, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comptaibility => compatibility

stof reviewed Sep 18, 2012
View reviewed changes

pborreli reviewed Sep 18, 2012
View reviewed changes

romainneutron mentioned this pull request Sep 18, 2012

[2.1][Process] Fix stop in non-sigchild environments #5543

Merged

romainneutron mentioned this pull request Oct 15, 2012

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

Closed

fabpot closed this Oct 27, 2012

pborreli mentioned this pull request Mar 1, 2013

[Process] Process doesn't start if calling script terminates to quickly #7237

Closed

fabpot reopened this Apr 9, 2013

fabpot mentioned this pull request Apr 9, 2013

Add a kill($signal) method to Process #5454

Closed

[Process] Add Process::getPid() method

a96a02a

fabpot reviewed Apr 10, 2013
View reviewed changes

romainneutron added 2 commits April 11, 2013 17:10

[Process] Add Process::signal() method

f453af6

Fix unit tests

e06c2a7

romainneutron mentioned this pull request Apr 14, 2013

[Process] Add documentation about signal and getPid methods symfony/symfony-docs#2504

Merged

fabpot closed this in 46efc9b Apr 14, 2013

romainneutron deleted the EnhanceSigchild branch April 14, 2013 16:45

WishCow mentioned this pull request Jun 3, 2013

[2.2] Console commands return with status 0, even if they fail #8180

Closed

Uh oh!

[Process] Add signal and getPid methods #5476

[Process] Add signal and getPid methods #5476

Uh oh!

Conversation

romainneutron commented Sep 9, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stof commented Sep 18, 2012

Uh oh!

romainneutron commented Sep 18, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romainneutron commented Sep 18, 2012

Uh oh!

marijn commented Sep 18, 2012

Uh oh!

stof commented Sep 18, 2012

Uh oh!

marijn commented Sep 18, 2012

Uh oh!

romainneutron commented Sep 18, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romainneutron commented Sep 18, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stof commented Sep 18, 2012

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!

fabpot commented Oct 4, 2012

Uh oh!

romainneutron commented Oct 4, 2012

Uh oh!

fabpot commented Oct 4, 2012

Uh oh!

stof commented Oct 4, 2012

Uh oh!

fabpot commented Oct 27, 2012

Uh oh!

fabpot commented Apr 9, 2013

Uh oh!

romainneutron commented Apr 9, 2013

Uh oh!

romainneutron commented Apr 9, 2013

Uh oh!

romainneutron commented Apr 9, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Apr 10, 2013

Uh oh!

romainneutron commented Apr 14, 2013

Uh oh!

romainneutron commented Apr 14, 2013

Uh oh!

Uh oh!