RFC: Zend Signal Handling

14 years ago by Stas Malyshev — view source

unread

Hi!

The patch is solid (have been using it in production for quite some
time) and improvement is quite helpful, especially when APC is being
used. Are there any reasons not to apply this to 5.4?

I don't know of any. Are there any issues with this change (BC, etc.)?

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

14 years ago by Ilia Alshanetsky — view source

unread

I do not believe so.

Hi!

The patch is solid (have been using it in production for quite some
time) and improvement is quite helpful, especially when APC is being
used. Are there any reasons not to apply this to 5.4?

I don't know of any. Are there any issues with this change (BC, etc.)?

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

14 years ago by Stas Malyshev — view source

unread

Hi!

I do not believe so.

Then I guess if nobody has any objections we can do it.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

14 years ago by Rasmus — view source

unread

Hi!

The patch is solid (have been using it in production for quite some
time) and improvement is quite helpful, especially when APC is being
used. Are there any reasons not to apply this to 5.4?

I don't know of any. Are there any issues with this change (BC, etc.)?

There could be some weird interactivity issues with certain
environments, but the patch takes care of most issues and I think the
added stability this gives opcode caches is worth the minor risk.

-Rasmus

14 years ago by Pierre Joye — view source

unread

hi Ilia,

I would suggest to kill the TSRMLS_FETCH while being at it. They are
horribly slow and a couple of them can be replaced by the
TSRMLS_DC/CC, if I'm not mistaken.

For the windows side, I do not have the time to do the equivalent, so
if you commit the patch to trunk first so I can fix the build
accordingly and then merge, that would be easier for me :).

Cheers,

Since we are on the topic of reviewing past RFCs for 5.4, can we take
another look at the Zend Signals RFC:

https://wiki.php.net/rfc/zendsignals

The patch is solid (have been using it in production for quite some
time) and improvement is quite helpful, especially when APC is being
used. Are there any reasons not to apply this to 5.4?

--

--
Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

14 years ago by Ilia Alshanetsky — view source

unread

Killing TSRMLS_FETCH is a noble goal, but let's keep it to once patch
at a time please ;-) And for the record I am all for killing
TSRMLS_FETCH.

hi Ilia,

I would suggest to kill the TSRMLS_FETCH while being at it. They are
horribly slow and a couple of them can be replaced by the
TSRMLS_DC/CC, if I'm not mistaken.

For the windows side, I do not have the time to do the equivalent, so
if you commit the patch to trunk first so I can fix the build
accordingly and then merge, that would be easier for me :).

Cheers,

Since we are on the topic of reviewing past RFCs for 5.4, can we take
another look at the Zend Signals RFC:

https://wiki.php.net/rfc/zendsignals

The patch is solid (have been using it in production for quite some
time) and improvement is quite helpful, especially when APC is being
used. Are there any reasons not to apply this to 5.4?

--

--
Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

14 years ago by Pierre Joye — view source

unread

Killing TSRMLS_FETCH is a noble goal, but let's keep it to once patch
at a time please ;-)

I mean in this patch only. This patch adds a couple, so it can be done
at the same time (afair these functions are not used heavily outside
the engine).

And for the record I am all for killing
TSRMLS_FETCH.

Same here (tls and some other ideas may help :)

hi Ilia,

I would suggest to kill the TSRMLS_FETCH while being at it. They are
horribly slow and a couple of them can be replaced by the
TSRMLS_DC/CC, if I'm not mistaken.

For the windows side, I do not have the time to do the equivalent, so
if you commit the patch to trunk first so I can fix the build
accordingly and then merge, that would be easier for me :).

Cheers,

Since we are on the topic of reviewing past RFCs for 5.4, can we take
another look at the Zend Signals RFC:

https://wiki.php.net/rfc/zendsignals

The patch is solid (have been using it in production for quite some
time) and improvement is quite helpful, especially when APC is being
used. Are there any reasons not to apply this to 5.4?

--

--
Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

--
Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

14 years ago by Gustavo Lopes — view source

unread

Em Thu, 02 Jun 2011 18:10:50 +0100, Ilia Alshanetsky ilia@prohost.org
escreveu:

Killing TSRMLS_FETCH is a noble goal, but let's keep it to once patch
at a time please ;-) And for the record I am all for killing
TSRMLS_FETCH.

Is there any advantage in killing it as opposed to simply not use it?

You will be breaking a lot of extensions. Some of them depend on libraries
with functions that receive callbacks to which you cannot pass specific
user data, which unnecessarily complicates refactoring.

--
Gustavo Lopes

14 years ago by Michael Maclean — view source

unread

Em Thu, 02 Jun 2011 18:10:50 +0100, Ilia Alshanetsky ilia@prohost.org
escreveu:

Killing TSRMLS_FETCH is a noble goal, but let's keep it to once patch
at a time please ;-) And for the record I am all for killing
TSRMLS_FETCH.

Is there any advantage in killing it as opposed to simply not use it?

I think he meant just replacing it in this patch.

--
Cheers,
Michael

14 years ago by Felipe Pena — view source

unread

Hi,

2011/6/2 Michael Maclean michael@no-surprises.co.uk

Em Thu, 02 Jun 2011 18:10:50 +0100, Ilia Alshanetsky ilia@prohost.org
escreveu:

Killing TSRMLS_FETCH is a noble goal, but let's keep it to once patch

at a time please ;-) And for the record I am all for killing
TSRMLS_FETCH.

Is there any advantage in killing it as opposed to simply not use it?

I think he meant just replacing it in this patch.

Just to inform, with the patched applied in trunk we have 4 SIGSEGVs with
ext/pcntl tests:

pcntl_alarm() [ext/pcntl/tests/pcntl_alarm.phpt]
pcntl_signal() [ext/pcntl/tests/pcntl_signal.phpt]
pcnt_signal_dispatch() [ext/pcntl/tests/pcntl_signal_dispatch.phpt]
Closures as a signal handler [ext/pcntl/tests/signal_closure_handler.phpt]

And 1 test hanging:
ext/pcntl/tests/002.phpt

--
Regards,
Felipe Pena

14 years ago by Felipe Pena — view source

unread

2011/6/2 Felipe Pena felipensp@gmail.com

Hi,

2011/6/2 Michael Maclean michael@no-surprises.co.uk

Em Thu, 02 Jun 2011 18:10:50 +0100, Ilia Alshanetsky ilia@prohost.org
escreveu:

Killing TSRMLS_FETCH is a noble goal, but let's keep it to once patch

at a time please ;-) And for the record I am all for killing
TSRMLS_FETCH.

Is there any advantage in killing it as opposed to simply not use it?

I think he meant just replacing it in this patch.

Just to inform, with the patched applied in trunk we have 4 SIGSEGVs with
ext/pcntl tests:

pcntl_alarm() [ext/pcntl/tests/pcntl_alarm.phpt]
pcntl_signal() [ext/pcntl/tests/pcntl_signal.phpt]
pcnt_signal_dispatch() [ext/pcntl/tests/pcntl_signal_dispatch.phpt]
Closures as a signal handler [ext/pcntl/tests/signal_closure_handler.phpt]

And 1 test hanging:
ext/pcntl/tests/002.phpt

Ok, already fixed. There is only a test failing due a behavior change:

$ cat ext/pcntl/tests/pcntl_signal.diff
009+ Fatal error: Error installing signal handler for -1 in
/home/felipe/dev/phptrunk/ext/pcntl/tests/pcntl_signal.php on line 10
009- Warning: pcntl_signal(): Error assigning signal %s
010- bool(false)
011-
012- Warning: pcntl_signal(): Error assigning signal %s
013- bool(false)
014-
015- Warning: pcntl_signal(): not callable is not a callable function name
error in %s
016- bool(false)
017- ok

--
Regards,
Felipe Pena

14 years ago by Ilia Alshanetsky — view source

unread

The crash is now fixed as well.

2011/6/2 Felipe Pena felipensp@gmail.com

Hi,

2011/6/2 Michael Maclean michael@no-surprises.co.uk

Em Thu, 02 Jun 2011 18:10:50 +0100, Ilia Alshanetsky ilia@prohost.org
escreveu:

Killing TSRMLS_FETCH is a noble goal, but let's keep it to once patch

at a time please ;-) And for the record I am all for killing
TSRMLS_FETCH.

Is there any advantage in killing it as opposed to simply not use it?

I think he meant just replacing it in this patch.

Just to inform, with the patched applied in trunk we have 4 SIGSEGVs with
ext/pcntl tests:

pcntl_alarm() [ext/pcntl/tests/pcntl_alarm.phpt]
pcntl_signal() [ext/pcntl/tests/pcntl_signal.phpt]
pcnt_signal_dispatch() [ext/pcntl/tests/pcntl_signal_dispatch.phpt]
Closures as a signal handler [ext/pcntl/tests/signal_closure_handler.phpt]

And 1 test hanging:
ext/pcntl/tests/002.phpt

Ok, already fixed. There is only a test failing due a behavior change:

$ cat ext/pcntl/tests/pcntl_signal.diff
009+ Fatal error: Error installing signal handler for -1 in
/home/felipe/dev/phptrunk/ext/pcntl/tests/pcntl_signal.php on line 10
009- Warning: pcntl_signal(): Error assigning signal %s
010- bool(false)
011-
012- Warning: pcntl_signal(): Error assigning signal %s
013- bool(false)
014-
015- Warning: pcntl_signal(): not callable is not a callable function name
error in %s
016- bool(false)
017- ok

--
Regards,
Felipe Pena

14 years ago by Felipe Pena — view source

unread

Fixed invalid sigaction() call passing NSIG as signal number.

for (signo = 1; signo <= NSIG; ++signo) {

for (signo = 1; signo < NSIG; ++signo) {

Detected by Valgrind:
==4577== Warning: bad signal number 65 in sigaction()

2011/6/3 Ilia Alshanetsky ilia@prohost.org

The crash is now fixed as well.

2011/6/2 Felipe Pena felipensp@gmail.com

Hi,

2011/6/2 Michael Maclean michael@no-surprises.co.uk

Em Thu, 02 Jun 2011 18:10:50 +0100, Ilia Alshanetsky <
ilia@prohost.org>
escreveu:

Killing TSRMLS_FETCH is a noble goal, but let's keep it to once patch

at a time please ;-) And for the record I am all for killing
TSRMLS_FETCH.

Is there any advantage in killing it as opposed to simply not use it?

I think he meant just replacing it in this patch.

Just to inform, with the patched applied in trunk we have 4 SIGSEGVs
with
ext/pcntl tests:

pcntl_alarm() [ext/pcntl/tests/pcntl_alarm.phpt]
pcntl_signal() [ext/pcntl/tests/pcntl_signal.phpt]
pcnt_signal_dispatch() [ext/pcntl/tests/pcntl_signal_dispatch.phpt]
Closures as a signal handler
[ext/pcntl/tests/signal_closure_handler.phpt]

And 1 test hanging:
ext/pcntl/tests/002.phpt

Ok, already fixed. There is only a test failing due a behavior change:

$ cat ext/pcntl/tests/pcntl_signal.diff
009+ Fatal error: Error installing signal handler for -1 in
/home/felipe/dev/phptrunk/ext/pcntl/tests/pcntl_signal.php on line 10
009- Warning: pcntl_signal(): Error assigning signal %s
010- bool(false)
011-
012- Warning: pcntl_signal(): Error assigning signal %s
013- bool(false)
014-
015- Warning: pcntl_signal(): not callable is not a callable function
name
error in %s
016- bool(false)
017- ok

--
Regards,
Felipe Pena

--
Regards,
Felipe Pena

14 years ago by Felipe Pena — view source

unread

Fixed crash in fastcgi due startup order...

SIGG() were being used before tsrm_startup().

2011/6/4 Felipe Pena felipensp@gmail.com

Fixed invalid sigaction() call passing NSIG as signal number.

for (signo = 1; signo <= NSIG; ++signo) {

for (signo = 1; signo < NSIG; ++signo) {

Detected by Valgrind:
==4577== Warning: bad signal number 65 in sigaction()

2011/6/3 Ilia Alshanetsky ilia@prohost.org

The crash is now fixed as well.

2011/6/2 Felipe Pena felipensp@gmail.com

Hi,

2011/6/2 Michael Maclean michael@no-surprises.co.uk

Em Thu, 02 Jun 2011 18:10:50 +0100, Ilia Alshanetsky <
ilia@prohost.org>
escreveu:

Killing TSRMLS_FETCH is a noble goal, but let's keep it to once
patch

at a time please ;-) And for the record I am all for killing
TSRMLS_FETCH.

Is there any advantage in killing it as opposed to simply not use it?

I think he meant just replacing it in this patch.

Just to inform, with the patched applied in trunk we have 4 SIGSEGVs
with
ext/pcntl tests:

pcntl_alarm() [ext/pcntl/tests/pcntl_alarm.phpt]
pcntl_signal() [ext/pcntl/tests/pcntl_signal.phpt]
pcnt_signal_dispatch() [ext/pcntl/tests/pcntl_signal_dispatch.phpt]
Closures as a signal handler
[ext/pcntl/tests/signal_closure_handler.phpt]

And 1 test hanging:
ext/pcntl/tests/002.phpt

Ok, already fixed. There is only a test failing due a behavior change:

$ cat ext/pcntl/tests/pcntl_signal.diff
009+ Fatal error: Error installing signal handler for -1 in
/home/felipe/dev/phptrunk/ext/pcntl/tests/pcntl_signal.php on line 10
009- Warning: pcntl_signal(): Error assigning signal %s
010- bool(false)
011-
012- Warning: pcntl_signal(): Error assigning signal %s
013- bool(false)
014-
015- Warning: pcntl_signal(): not callable is not a callable function
name
error in %s
016- bool(false)
017- ok

--
Regards,
Felipe Pena

--
Regards,
Felipe Pena

--
Regards,
Felipe Pena

14 years ago by Christopher Jones — view source

unread

Since we are on the topic of reviewing past RFCs for 5.4, can we take
another look at the Zend Signals RFC:

https://wiki.php.net/rfc/zendsignals

The patch is solid (have been using it in production for quite some
time) and improvement is quite helpful, especially when APC is being
used. Are there any reasons not to apply this to 5.4?

Can you post an updated patch?

Chris

--
Email: christopher.jones@oracle.com
Tel: +1 650 506 8630
Blog: http://blogs.oracle.com/opal/