Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Process] Enhance compatiblity with --enable-sigchild#16915
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
why changing these conditions ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
because there are too much tightened to implementation details, and in fact do not work when testing with a sighchild enabled php
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
we are now able to retrieve it when enhancing compat, but can we also without enhancing it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
knowing "if" it has been signaled works with or without enhanced mode thanks tohttps://github.com/symfony/symfony/pull/16915/files#diff-f9f2411040cda7b73402481facf3e4ddR1133
knowing which signal is more tricky and requires enhanced mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
in fact, I was wrong, knowing "if" it has been signaled doesn't work without the enhanced mode, fixed
14d13ee to7b11f1fComparenicolas-grekas commentedDec 8, 2015
Comments addressed, ready for a second round of review :) |
Seldaek commentedDec 8, 2015
Sorry not able to review in depth right now but I sure hope it won't blow up anything. Then again I think sigchild usage is probably less and less of a problem the more time passes. |
8cadae4 toe36b1b4Comparenicolas-grekas commentedDec 9, 2015
I added a special build of php with sigchild enabled on travis so that we can test, verify, prove and enforce that this really works :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
updateStatus is already called by requireProcessIsTerminated
b2d9c18 tobd38abdCompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I assume that the version will have to change for the 3.0 branch ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yes, I propose 5.5.9 there
stof commentedDec 9, 2015
@nicolas-grekas have you tried running tests when forcing the sighchild compat layer to be disabled ? |
nicolas-grekas commentedDec 9, 2015
@stof yes. Nothing interesting here: tests either fail because of one of those exceptions about --enable-sigchild, or pass. |
stof commentedDec 9, 2015
@nicolas-grekas but it needs to be done to ensure we don't miss some of the sigchild checks |
nicolas-grekas commentedDec 9, 2015 via email
Sigchild is now tested with and without the enhanced mode. |
romainneutron commentedDec 10, 2015
Looks good to me |
070bf65 todb6239dComparedb6239d toe7cc4aaCompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
aren't you missing aI in the name ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
SIGCHLD is the name of the said signal, so it was intentional...
stof commentedDec 10, 2015
except for the comment I made, 👍 |
…olas-grekas)This PR was merged into the 2.3 branch.Discussion----------[Process] Enhance compatiblity with --enable-sigchild| Q | A| ------------- | ---| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#16888| License | MIT| Doc PR | -This is complete rewrite of the fallback `--enable-sigchild` handling in the Process class.It removes most of the differences between this and a non-sigchild-enabled php.Which means the test suite doesn't need anymore to be replayed 3 times (which is how I started this PR, looking for a way to test this component in less time).I validated this with a locally compiled php, sigchild-enabled. Green.Changes affect only this special-mode php.Ping@romainneutron and@Seldaek (original writer of the sigchild support)Submitted on 2.3 as bugfix, which it is to me.Commits-------e7cc4aa [Process] Enhance compatiblity with --enable-sigchild
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
A comment explaining what's done in this commandline, with a link to the stackoverflow issue would be more than welcome.
I doubt anybody could read this without error in two years (thinking about the first implementation was years ago)
This is complete rewrite of the fallback
--enable-sigchildhandling in the Process class.It removes most of the differences between this and a non-sigchild-enabled php.
Which means the test suite doesn't need anymore to be replayed 3 times (which is how I started this PR, looking for a way to test this component in less time).
I validated this with a locally compiled php, sigchild-enabled. Green.
Changes affect only this special-mode php.
Ping@romainneutron and@Seldaek (original writer of the sigchild support)
Submitted on 2.3 as bugfix, which it is to me.