# RFC: Zend Signal Handling - Thread: https://externals.io/message/52606 - Original archive: https://news-web.php.net/php.internals/52606 - Started by Ilia Alshanetsky on 2011-05-31T22:30:06+00:00 - Messages: 16 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #52606 — Ilia Alshanetsky — 2011-05-31T22:30:06+00:00 URL: https://externals.io/message/52606#52606 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? --- ## #52607 — Stas Malyshev — 2011-05-31T22:35:28+00:00 URL: https://externals.io/message/52606#52607 · In reply to: #52606 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 --- ## #52608 — Ilia Alshanetsky — 2011-05-31T22:37:48+00:00 URL: https://externals.io/message/52606#52608 · In reply to: #52607 I do not believe so. > Hi! > > > […] > > 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 --- ## #52609 — Stas Malyshev — 2011-05-31T22:39:51+00:00 URL: https://externals.io/message/52606#52609 · In reply to: #52608 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 --- ## #52611 — Rasmus — 2011-05-31T23:01:18+00:00 URL: https://externals.io/message/52606#52611 · In reply to: #52607 > Hi! > > > […] > > 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 --- ## #52791 — Pierre Joye — 2011-06-02T16:04:32+00:00 URL: https://externals.io/message/52606#52791 · In reply to: #52606 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 --- ## #52795 — Christopher Jones — 2011-06-02T17:02:09+00:00 URL: https://externals.io/message/52606#52795 · In reply to: #52606 > 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/ --- ## #52796 — Ilia Alshanetsky — 2011-06-02T17:10:50+00:00 URL: https://externals.io/message/52606#52796 · In reply to: #52791 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, > > > > […] > > > > -- > Pierre > > @pierrejoye | http://blog.thepimp.net | http://www.libgd.org --- ## #52798 — Pierre Joye — 2011-06-02T17:17:56+00:00 URL: https://externals.io/message/52606#52798 · In reply to: #52796 > 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 :) > > > […] > -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org --- ## #52799 — Gustavo Lopes — 2011-06-02T17:20:51+00:00 URL: https://externals.io/message/52606#52799 · In reply to: #52796 Em Thu, 02 Jun 2011 18:10:50 +0100, Ilia Alshanetsky 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 --- ## #52822 — Michael Maclean — 2011-06-02T22:19:35+00:00 URL: https://externals.io/message/52606#52822 · In reply to: #52799 > Em Thu, 02 Jun 2011 18:10:50 +0100, Ilia Alshanetsky > escreveu: > > > […] > > 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 --- ## #52827 — Felipe Pena — 2011-06-03T00:19:44+00:00 URL: https://externals.io/message/52606#52827 · In reply to: #52822 Hi, 2011/6/2 Michael Maclean > > > > […] > > 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 --- ## #52828 — Felipe Pena — 2011-06-03T00:41:49+00:00 URL: https://externals.io/message/52606#52828 · In reply to: #52827 2011/6/2 Felipe Pena > Hi, > > 2011/6/2 Michael Maclean > > > […] > 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 --- ## #52866 — Ilia Alshanetsky — 2011-06-03T17:22:35+00:00 URL: https://externals.io/message/52606#52866 · In reply to: #52828 The crash is now fixed as well. > 2011/6/2 Felipe Pena > > > […] > > > 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 --- ## #52907 — Felipe Pena — 2011-06-05T01:17:58+00:00 URL: https://externals.io/message/52606#52907 · In reply to: #52866 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 > The crash is now fixed as well. > > > > […] > ilia@prohost.org> > > […] > with > > […] > [ext/pcntl/tests/signal_closure_handler.phpt] > > […] > name > > […] > -- Regards, Felipe Pena --- ## #52948 — Felipe Pena — 2011-06-05T17:09:14+00:00 URL: https://externals.io/message/52606#52948 · In reply to: #52907 Fixed crash in fastcgi due startup order... SIGG() were being used before tsrm_startup(). 2011/6/4 Felipe Pena > 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 > > > […] > > > > -- > Regards, > Felipe Pena > -- Regards, Felipe Pena