Re: [PHP-DEV] RFC: Zend Signal Handling
Fixed crash in fastcgi due startup order...
SIGG() were being used before tsrm_startup().
2011/6/4 Felipe Pena felipe...@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 i...@prohost.org
The crash is now fixed as well.
On Fri, Jun 3, 2011 at 2:41 AM, Felipe Pena felipe...@gmail.com wrote:
2011/6/2 Felipe Pena felipe...@gmail.com
Hi,
2011/6/2 Michael Maclean mich...@no-surprises.co.uk
On 02/06/11 18:20, Gustavo Lopes wrote:
Em Thu, 02 Jun 2011 18:10:50 +0100, Ilia Alshanetsky
i...@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
Re: [PHP-DEV] RFC: Zend Signal Handling
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 i...@prohost.org
The crash is now fixed as well.
On Fri, Jun 3, 2011 at 2:41 AM, Felipe Pena felipe...@gmail.com wrote:
2011/6/2 Felipe Pena felipe...@gmail.com
Hi,
2011/6/2 Michael Maclean mich...@no-surprises.co.uk
On 02/06/11 18:20, Gustavo Lopes wrote:
Em Thu, 02 Jun 2011 18:10:50 +0100, Ilia Alshanetsky
i...@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
Re: [PHP-DEV] RFC: Zend Signal Handling
The crash is now fixed as well. On Fri, Jun 3, 2011 at 2:41 AM, Felipe Pena felipe...@gmail.com wrote: 2011/6/2 Felipe Pena felipe...@gmail.com Hi, 2011/6/2 Michael Maclean mich...@no-surprises.co.uk On 02/06/11 18:20, Gustavo Lopes wrote: Em Thu, 02 Jun 2011 18:10:50 +0100, Ilia Alshanetsky i...@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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] RFC: Zend Signal Handling
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, On Wed, Jun 1, 2011 at 12:30 AM, Ilia Alshanetsky i...@prohost.org wrote: 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? -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] RFC: Zend Signal Handling
On 05/31/2011 03:30 PM, Ilia Alshanetsky wrote: 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.jo...@oracle.com Tel: +1 650 506 8630 Blog: http://blogs.oracle.com/opal/ -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] RFC: Zend Signal Handling
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. On Thu, Jun 2, 2011 at 6:04 PM, Pierre Joye pierre@gmail.com wrote: 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, On Wed, Jun 1, 2011 at 12:30 AM, Ilia Alshanetsky i...@prohost.org wrote: 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? -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] RFC: Zend Signal Handling
On Thu, Jun 2, 2011 at 7:10 PM, Ilia Alshanetsky i...@prohost.org wrote: 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 :) On Thu, Jun 2, 2011 at 6:04 PM, Pierre Joye pierre@gmail.com wrote: 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, On Wed, Jun 1, 2011 at 12:30 AM, Ilia Alshanetsky i...@prohost.org wrote: 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? -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] RFC: Zend Signal Handling
Em Thu, 02 Jun 2011 18:10:50 +0100, Ilia Alshanetsky i...@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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] RFC: Zend Signal Handling
On 02/06/11 18:20, Gustavo Lopes wrote: Em Thu, 02 Jun 2011 18:10:50 +0100, Ilia Alshanetsky i...@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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] RFC: Zend Signal Handling
Hi, 2011/6/2 Michael Maclean mich...@no-surprises.co.uk On 02/06/11 18:20, Gustavo Lopes wrote: Em Thu, 02 Jun 2011 18:10:50 +0100, Ilia Alshanetsky i...@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
Re: [PHP-DEV] RFC: Zend Signal Handling
2011/6/2 Felipe Pena felipe...@gmail.com Hi, 2011/6/2 Michael Maclean mich...@no-surprises.co.uk On 02/06/11 18:20, Gustavo Lopes wrote: Em Thu, 02 Jun 2011 18:10:50 +0100, Ilia Alshanetsky i...@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
[PHP-DEV] RFC: Zend Signal Handling
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? -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] RFC: Zend Signal Handling
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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] RFC: Zend Signal Handling
I do not believe so. On Wed, Jun 1, 2011 at 12:35 AM, Stas Malyshev smalys...@sugarcrm.com wrote: 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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] RFC: Zend Signal Handling
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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] RFC: Zend Signal Handling
On 05/31/2011 03:35 PM, Stas Malyshev wrote: 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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
How shall we continue with this patch? Arnaud, I need an updated HEAD patch to match the changes made in 5.3. There's valid concern about performance in ZTS, which seems hard to address without the __thread changes. It seems you are working on to get this into 6.0. Dmitry, Lukas, Arnaud and others interested, would it make sense to proceed with the current non-ZTS signals implementation in 5.3 and work on ZTS in HEAD with a goal to improve performance? -lucas On 8/14/08 4:22 PM, Arnaud Le Blanc [EMAIL PROTECTED] wrote: On Thursday 14 August 2008 02:59:48 Arnaud Le Blanc wrote: Hi, I made some changes to the patch: http://arnaud.lb.s3.amazonaws.com/php-5.3.0-alarms-0808141122.patch - Apache effectively seems to resets the signals after MINIT, so original handlers are now saved in RINIT in the first request of the process. - Removed some unneeded blocks/unblocks. - Changed all added TSRMLS_FETCH() so that they affect only ZTS+ZEND_SIGNAL builds. - Removed some added TSRMLS_FETCH() (all in zend_alloc and a few in zend_hash). Regards, Arnaud I made an other patch that caches tsrm_ls in a thread-local variable when calling TSRMLS_FETCH(), on platforms that support the __thread specifier: http://arnaud.lb.s3.amazonaws.com/tsrm_ls_cache.patch This gives a 10 to 15% speedup to ZTS builds on bench.php. It also makes ZEND_SIGNAL+ZTS builds run as fast as ZTS without ZEND_SIGNAL. Regards, Arnaud -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
On Thursday 14 August 2008 02:59:48 Arnaud Le Blanc wrote: Hi, I made some changes to the patch: http://arnaud.lb.s3.amazonaws.com/php-5.3.0-alarms-0808141122.patch - Apache effectively seems to resets the signals after MINIT, so original handlers are now saved in RINIT in the first request of the process. - Removed some unneeded blocks/unblocks. - Changed all added TSRMLS_FETCH() so that they affect only ZTS+ZEND_SIGNAL builds. - Removed some added TSRMLS_FETCH() (all in zend_alloc and a few in zend_hash). Regards, Arnaud I made an other patch that caches tsrm_ls in a thread-local variable when calling TSRMLS_FETCH(), on platforms that support the __thread specifier: http://arnaud.lb.s3.amazonaws.com/tsrm_ls_cache.patch This gives a 10 to 15% speedup to ZTS builds on bench.php. It also makes ZEND_SIGNAL+ZTS builds run as fast as ZTS without ZEND_SIGNAL. Regards, Arnaud -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
Hi Lucas, I took a look into patch and I still don't like it. I may miss some things and make mistakes so correct me if I'm wrong. 1) It makes some slowdown for all SAPIs except Apache1, because it adds additional block/unblock code (mainly in zend_alloc.c) 2) It makes ~10 additional syscalls on each request. I didn't get why do you save original handlers in MINIT and set new handlers in RINIT. In general they can be changed between MINIT and RINIT by other apache module or web server so they should be saved/set in RINIT, but from performance point of view it might be better to save/set signals in MINIT. 3) The patch cannot solve all issues caused by SIGALARM handling, because PHP has thousand places which may stay data in inconsistent state (or data allocated in permanent heap). Wrapping all of them with block/unblock is not a decision. small issues: 4) block/unblock code in estrndup() and some other functions is useless. It should be removed. 5) TSRM_FETCH() in zend_hash and zend_alloc functions will slowdown Windows ZTS build even if it doesn't use signals. It may be changed to some other macro that will do TSRM_FETCH only if signals are enabled. May be if we really like safe SIGALARM handling we may use Windows decision for UNIX too. I mean setup of EG(timed_out) flag in signal handler and checking it in execute loop. As an optimization we can check it not on each instruction but only on enter in function and JMP* opcodes (loops). Anyway, this solution may cause comparable slowdown. Thanks. Dmitry. Lucas Nealan wrote: On 8/11/08 8:52 AM, Dmitry Stogov [EMAIL PROTECTED] wrote: I'll try to review it on Tuesday/Wednesday. Thanks. Dmitry. I've just updated the patches. Only some very minor changes as discussed before and they should cleanly apply against current cvs. -lucas -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
Dmitry Stogov wrote: Hi Lucas, I took a look into patch and I still don't like it. I may miss some things and make mistakes so correct me if I'm wrong. 1) It makes some slowdown for all SAPIs except Apache1, because it adds additional block/unblock code (mainly in zend_alloc.c) This is the part that has never made sense to me. If we don't need to block signals in zend_alloc, remove those HANDLE_[UN]BLOCK_INTERRUPTIONS macros. And since these are a NOP for all but the Apache1 SAPI, either all other sapis are broken, or these are not needed at all. 2) It makes ~10 additional syscalls on each request. I didn't get why do you save original handlers in MINIT and set new handlers in RINIT. In general they can be changed between MINIT and RINIT by other apache module or web server so they should be saved/set in RINIT, but from performance point of view it might be better to save/set signals in MINIT. One of the issues here is that Apache sets its own signals after calling all the MINIT hooks. So saving the signals in the MINIT doesn't work. It has to be in the RINIT, but it could theoretically be done just on the first request although there are issue with that too. None of this is pretty, but completely ignoring all our signal issues doesn't seem like a good idea either. -Rasmus -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
Hi Rasmus, Rasmus Lerdorf wrote: Dmitry Stogov wrote: Hi Lucas, I took a look into patch and I still don't like it. I may miss some things and make mistakes so correct me if I'm wrong. 1) It makes some slowdown for all SAPIs except Apache1, because it adds additional block/unblock code (mainly in zend_alloc.c) This is the part that has never made sense to me. If we don't need to block signals in zend_alloc, remove those HANDLE_[UN]BLOCK_INTERRUPTIONS macros. And since these are a NOP for all but the Apache1 SAPI, either all other sapis are broken, or these are not needed at all. It is a question. Of course this block/unblock code prevents staying the heap in inconsistent state which can cause SIGSEGV later and new code must be safer, but slower. :( If we remove all HANDLE_[UN]BLOCK_INTERRUPTIONS we don't need this patch at all. :) 2) It makes ~10 additional syscalls on each request. I didn't get why do you save original handlers in MINIT and set new handlers in RINIT. In general they can be changed between MINIT and RINIT by other apache module or web server so they should be saved/set in RINIT, but from performance point of view it might be better to save/set signals in MINIT. One of the issues here is that Apache sets its own signals after calling all the MINIT hooks. So saving the signals in the MINIT doesn't work. It has to be in the RINIT, but it could theoretically be done just on the first request although there are issue with that too. None of this is pretty, but completely ignoring all our signal issues doesn't seem like a good idea either. First request is a good idea. Thanks. Dmitry. -Rasmus -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
Dmitry Stogov wrote: Hi Rasmus, Rasmus Lerdorf wrote: Dmitry Stogov wrote: Hi Lucas, I took a look into patch and I still don't like it. I may miss some things and make mistakes so correct me if I'm wrong. 1) It makes some slowdown for all SAPIs except Apache1, because it adds additional block/unblock code (mainly in zend_alloc.c) This is the part that has never made sense to me. If we don't need to block signals in zend_alloc, remove those HANDLE_[UN]BLOCK_INTERRUPTIONS macros. And since these are a NOP for all but the Apache1 SAPI, either all other sapis are broken, or these are not needed at all. It is a question. Of course this block/unblock code prevents staying the heap in inconsistent state which can cause SIGSEGV later and new code must be safer, but slower. :( If we remove all HANDLE_[UN]BLOCK_INTERRUPTIONS we don't need this patch at all. :) That assumes no extensions need critical sections, which is a bad assumption, especially for anything playing with shared memory like opcode caches. -Rasmus -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
I wasn't serious about removing them. Anyway zend_alloc.c and zend_hash.c don't work with shared memory. It's possible to reuse zend_alloc.c to allocate blocks from SHM, but it's not its primary purpose. If APC don't do it I don't see how this block/unblock code may protect SHM. It definitely may protect PHP from crashes and memory corruptions during RSHUTDOWN, but only for some cases. Thanks. Dmitry. Rasmus Lerdorf wrote: Dmitry Stogov wrote: Hi Rasmus, Rasmus Lerdorf wrote: Dmitry Stogov wrote: Hi Lucas, I took a look into patch and I still don't like it. I may miss some things and make mistakes so correct me if I'm wrong. 1) It makes some slowdown for all SAPIs except Apache1, because it adds additional block/unblock code (mainly in zend_alloc.c) This is the part that has never made sense to me. If we don't need to block signals in zend_alloc, remove those HANDLE_[UN]BLOCK_INTERRUPTIONS macros. And since these are a NOP for all but the Apache1 SAPI, either all other sapis are broken, or these are not needed at all. It is a question. Of course this block/unblock code prevents staying the heap in inconsistent state which can cause SIGSEGV later and new code must be safer, but slower. :( If we remove all HANDLE_[UN]BLOCK_INTERRUPTIONS we don't need this patch at all. :) That assumes no extensions need critical sections, which is a bad assumption, especially for anything playing with shared memory like opcode caches. -Rasmus -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
On 8/13/08 1:09 AM, Dmitry Stogov [EMAIL PROTECTED] wrote: I took a look into patch and I still don't like it. I may miss some things and make mistakes so correct me if I'm wrong. 1) It makes some slowdown for all SAPIs except Apache1, because it adds additional block/unblock code (mainly in zend_alloc.c) The additional code was added by Arnaud as he added ZTS support. I'll let him comment as I did not spend any time in the allocator determining if we should add additional critical sections. Technically there are *many* sections of startup where it would make sense to add critical sections, however the likelyhood of getting a signal here is extremely low. 2) It makes ~10 additional syscalls on each request. I didn't get why do you save original handlers in MINIT and set new handlers in RINIT. In general they can be changed between MINIT and RINIT by other apache module or web server so they should be saved/set in RINIT, but from performance point of view it might be better to save/set signals in MINIT. Well I can never be sure of what the sapi is doing in regards to it's signal handling. In apache, for example it resets some of it's signal handlers at the beginning of each request. It's possible that we may be able to reduce the number of these in the apache instance but I wouldn't know what to do in any other sapi, thus the safest route was chosen. 3) The patch cannot solve all issues caused by SIGALARM handling, because PHP has thousand places which may stay data in inconsistent state (or data allocated in permanent heap). Wrapping all of them with block/unblock is not a decision. I don't have anything to add to this observation. My greatest concern is critical sections within the allocator where we do commonly see timeouts and possibly other signals and the critical sectios in APC. When our shared memory locks are frozen because APC relies on critical section protection, all requests can be locked forever. We may also see general corruption here. I would recommend against ever adding APC to core without some sort of protection such as what this patch offers, if that is still the eventual plan. 4) block/unblock code in estrndup() and some other functions is useless. It should be removed. 5) TSRM_FETCH() in zend_hash and zend_alloc functions will slowdown Windows ZTS build even if it doesn't use signals. It may be changed to some other macro that will do TSRM_FETCH only if signals are enabled. I'll let Arnaud respond to these as this is his portion of the patch. #5 may make sense to me though. May be if we really like safe SIGALARM handling we may use Windows decision for UNIX too. I mean setup of EG(timed_out) flag in signal handler and checking it in execute loop. As an optimization we can check it not on each instruction but only on enter in function and JMP* opcodes (loops). Anyway, this solution may cause comparable slowdown. In ZTS this may definitely be true, in non-zts it should not be any slower barring the added blocking or TSRM_FETCH's. These functions should be passed TSRM data and not have to fetch, which I assume would perform better. Arnaud, do you have any performance comparisons before/after the patch in ZTS? -lucas -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
On 8/13/08 2:10 AM, Dmitry Stogov [EMAIL PROTECTED] wrote: I wasn't serious about removing them. Anyway zend_alloc.c and zend_hash.c don't work with shared memory. It's possible to reuse zend_alloc.c to allocate blocks from SHM, but it's not its primary purpose. If APC don't do it I don't see how this block/unblock code may protect SHM. It definitely may protect PHP from crashes and memory corruptions during RSHUTDOWN, but only for some cases. APC directly uses the HANDLE_BLOCK/UNBLOCK_INTERRUPTIONS macros, I assume other extensions do this as well. -lucas -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
now I see the main reason of the patch, but while APC is not a part of PHP yet, it might be better, easer and safer to solve its issue in APC itself (this is just a though, not a recommendation). Anyway we need to make this patch better if we commit it, and I would ask other community members to look into it and share their opinions. Thanks. Dmitry. Lucas Nealan wrote: On 8/13/08 2:10 AM, Dmitry Stogov [EMAIL PROTECTED] wrote: I wasn't serious about removing them. Anyway zend_alloc.c and zend_hash.c don't work with shared memory. It's possible to reuse zend_alloc.c to allocate blocks from SHM, but it's not its primary purpose. If APC don't do it I don't see how this block/unblock code may protect SHM. It definitely may protect PHP from crashes and memory corruptions during RSHUTDOWN, but only for some cases. APC directly uses the HANDLE_BLOCK/UNBLOCK_INTERRUPTIONS macros, I assume other extensions do this as well. -lucas -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
Hi, On Wednesday 13 August 2008 10:09:43 Dmitry Stogov wrote: Hi Lucas, I took a look into patch and I still don't like it. I may miss some things and make mistakes so correct me if I'm wrong. 1) It makes some slowdown for all SAPIs except Apache1, because it adds additional block/unblock code (mainly in zend_alloc.c) The added block/unblocks are really needed, for instance in _zend_mm_alloc_int when a small free block is found, it has to maintain the cache, etc and an interruption here can cause some unwanted behaviors. 3) The patch cannot solve all issues caused by SIGALARM handling, because PHP has thousand places which may stay data in inconsistent state (or data allocated in permanent heap). Wrapping all of them with block/unblock is not a decision. small issues: 4) block/unblock code in estrndup() and some other functions is useless. It should be removed. I added that in estrndup() during testing as an interrupt in this function caused String is non-zero terminated warnings. 5) TSRM_FETCH() in zend_hash and zend_alloc functions will slowdown Windows ZTS build even if it doesn't use signals. It may be changed to some other macro that will do TSRM_FETCH only if signals are enabled. This can be done. For zend_alloc, as _emalloc already does TSRMLS_FETCH(), it is possible to pass the tsrm_ls directly as argument to _zend_mm_alloc_int(). _efree, _erealloc, etc do TSRMLS_FETCH() too. There is a ~5% slowdown on bench.php with ZTS builds with the patch applied. And ~3% when passing tsrm_ls to _zend_mm_alloc_int/free/realloc(). Passing the tsrm_ls to hash functions may avoid this slowdown but this requires huge changes. May be if we really like safe SIGALARM handling we may use Windows decision for UNIX too. I mean setup of EG(timed_out) flag in signal handler and checking it in execute loop. As an optimization we can check it not on each instruction but only on enter in function and JMP* opcodes (loops). Anyway, this solution may cause comparable slowdown. It may be a good solution too. Just removing #ifdefs around timed_out (so, without any optimization) gives a 1 to 2% slowdown. Regards, Arnaud -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
Hi, I made some changes to the patch: http://arnaud.lb.s3.amazonaws.com/php-5.3.0-alarms-0808141122.patch - Apache effectively seems to resets the signals after MINIT, so original handlers are now saved in RINIT in the first request of the process. - Removed some unneeded blocks/unblocks. - Changed all added TSRMLS_FETCH() so that they affect only ZTS+ZEND_SIGNAL builds. - Removed some added TSRMLS_FETCH() (all in zend_alloc and a few in zend_hash). Regards, Arnaud -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
Arnaud Le Blanc wrote: - Apache effectively seems to resets the signals after MINIT, so original handlers are now saved in RINIT in the first request of the process. Did you verify this behaviour with Apache2 as well? I've only checked Apache1. -Rasmus -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
Hi, On Thursday 14 August 2008 04:19:38 Rasmus Lerdorf wrote: Arnaud Le Blanc wrote: - Apache effectively seems to resets the signals after MINIT, so original handlers are now saved in RINIT in the first request of the process. Did you verify this behaviour with Apache2 as well? I've only checked Apache1. -Rasmus Yes, with the worker MPM it sets at least SIGHUP after MINIT. Regards, Arnaud -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
On 8/11/08 8:52 AM, Dmitry Stogov [EMAIL PROTECTED] wrote: I'll try to review it on Tuesday/Wednesday. Thanks. Dmitry. I've just updated the patches. Only some very minor changes as discussed before and they should cleanly apply against current cvs. -lucas -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
On 09.08.2008, at 17:42, Arnaud Le Blanc wrote: Hi, As Lucas said the patch seems ready now, could someone please review the patch for inclusion ? http://wiki.php.net/rfc/zendsignals Changes that have been made: - The patch has been ported to HEAD - The patch now supports multithreaded environments, and fixes many problems on non-windows platforms when enabled. - Extensions can use zend_signal() and zend_sigaction() so that all signals they declare will always be deferred in critical sections. PCNTL has been ported. - There is now a zend.signal_check ini entry so that the checks made in zend_signal_deactivate() can be done in non-debug builds. - Some code has been moved to process startup or global constructor routines. - The patch moves some HANDLE_BLOCK/UNBLOCK in zend_alloc to protect more code Yeah where do we stand with this patch? regards, Lukas Kahwe Smith [EMAIL PROTECTED] -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
I'll try to review it on Tuesday/Wednesday. Thanks. Dmitry. Lukas Kahwe Smith wrote: On 09.08.2008, at 17:42, Arnaud Le Blanc wrote: Hi, As Lucas said the patch seems ready now, could someone please review the patch for inclusion ? http://wiki.php.net/rfc/zendsignals Changes that have been made: - The patch has been ported to HEAD - The patch now supports multithreaded environments, and fixes many problems on non-windows platforms when enabled. - Extensions can use zend_signal() and zend_sigaction() so that all signals they declare will always be deferred in critical sections. PCNTL has been ported. - There is now a zend.signal_check ini entry so that the checks made in zend_signal_deactivate() can be done in non-debug builds. - Some code has been moved to process startup or global constructor routines. - The patch moves some HANDLE_BLOCK/UNBLOCK in zend_alloc to protect more code Yeah where do we stand with this patch? regards, Lukas Kahwe Smith [EMAIL PROTECTED] -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
Hi, As Lucas said the patch seems ready now, could someone please review the patch for inclusion ? http://wiki.php.net/rfc/zendsignals Changes that have been made: - The patch has been ported to HEAD - The patch now supports multithreaded environments, and fixes many problems on non-windows platforms when enabled. - Extensions can use zend_signal() and zend_sigaction() so that all signals they declare will always be deferred in critical sections. PCNTL has been ported. - There is now a zend.signal_check ini entry so that the checks made in zend_signal_deactivate() can be done in non-debug builds. - Some code has been moved to process startup or global constructor routines. - The patch moves some HANDLE_BLOCK/UNBLOCK in zend_alloc to protect more code Regards, Arnaud -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
Arnaud Le Blanc wrote: Hi, As Lucas said the patch seems ready now, could someone please review the patch for inclusion ? http://wiki.php.net/rfc/zendsignals Changes that have been made: - The patch has been ported to HEAD - The patch now supports multithreaded environments, and fixes many problems on non-windows platforms when enabled. - Extensions can use zend_signal() and zend_sigaction() so that all signals they declare will always be deferred in critical sections. PCNTL has been ported. - There is now a zend.signal_check ini entry so that the checks made in zend_signal_deactivate() can be done in non-debug builds. - Some code has been moved to process startup or global constructor routines. - The patch moves some HANDLE_BLOCK/UNBLOCK in zend_alloc to protect more code And don't forget that there was no protection before at all in any SAPI other than Apache1 because HANDLE_BLOCK_INTERRUPTIONS() is a NOP on every other SAPI. We effectively had no critial sections at all prior to this patch. -Rasmus -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
Votre message: Greetings! On 8/3/08 9:37 PM, Arnaud LB [EMAIL PROTECTED] wrote: If sigaction is not available Zend Signal Handling will not be enabled, so it will not be enabled on Windows (I assume sigaction is not available on Windows, it is ?). For pthreads and sigprocmask, tsrm_sigmask() can be improved to add support for other APIs. However it seems that only pthread provides that. Apache/APR relies on phtread_sigmask() when available, or sigprocmask() when not. Ahh correct. Wasn't looking at this correctly. As for non pthread sigprocmask sounds like we'll find out if it's problematic in any way. Good to know this is at least what apr is doing. If in the future we have anything in the stack registering a signal handler via zend_signal this will be problematic because SIGG_*_CRITICAL sections will not cover the signals registered post-startup. sigaction() is process-wide, so if a thread registers a signal post-startup (this is already the case, zend_sigs[] are registered at request-startup) the signal will be defered in all threads :) The deferring will basically work in the new model but I'm relying on sigprocmask(SIG_BLOCK/SIG_SETMASK) in the SIGNAL_CRITICAL macro's to protect against jumps during queue maintenance and in zend_signal_handler_unblock when it calls zend_signal_handler_defer. If a post startup signal is registered, the latest version of zend_signal doesn't add the new signal number to the mask, it only sets sa_mask to globals_sigmask. If we add the registered signal to the global_sigmask before setting sa_mask here then I think we would be ok. global_sigmask is initialized using sigfillset(), so it contains _all_ signals (except SIGSEGV, etc because non-blockable or not safe to block) and there is no need to add signals to global_sigmask it in zend_signal() / zend_signal_register() :) I originally did not use an array as I was hoping to dynamically allocate more queue at runtime. I hadn't fully thought it out because there are almost no calls into the code where this would be apropriate. We could maybe do something in zend_signal_handler_unblock but it might be too late there and with this it would not be possible. I don't think I have a problem with this though. Not that it won't happen but I can't imagine a scenario where we get hammered with more signals than we would have slots in ZEND_SIGNAL_QUEUE_SIZE and it wouldn't be safe to ignore them. At least with the signals we're handling out of the box. This could change when we get pcntl working with this. Yes, it may be great to be able to grow the queue when necessary. I'll make a note in the considerations section of the wiki that when we get zend_signal ready to hook up to pctnl we'll reconsider the static allocation. For extensions IMO we should just let them use zend_signal() as it currently is. Using pcntl_signal() will override handlers previously set by zend_signal() but the signal will remain deferred. I was also thinking to implement zend_sigaction(), allowing to know the previously registered signal, and to set some flags when registering signal handlers. -lucas -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
On 8/4/08 11:51 PM, Arnaud Le Blanc [EMAIL PROTECTED] wrote: global_sigmask is initialized using sigfillset(), so it contains _all_ signals (except SIGSEGV, etc because non-blockable or not safe to block) and there is no need to add signals to global_sigmask it in zend_signal() / zend_signal_register() :) Right you are. Apparently I saw it was doing that but it didn't really register. For extensions IMO we should just let them use zend_signal() as it currently is. Using pcntl_signal() will override handlers previously set by zend_signal() but the signal will remain deferred. I don't see how it would continue to be deferred. We'd be relying on the fact that pcntl depends on a tick or call to the dispatch function to actually invoke any userspace code. Once registering a signal via pcntl I'm able to jump out of a critical section into pctl_signal_handler when a signal is received. This code might be safe since it doesn't do very much but I don't get real protection. I updated the patches on the wiki: - Added zend_sigaction() - Ported PCNTL to use zend_sigaction() - Fixed handling of SIG_IGN Great. I'll pull these down. -lucas -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
On Tuesday 05 August 2008 12:28:19 Lucas Nealan wrote: On 8/4/08 11:51 PM, Arnaud Le Blanc [EMAIL PROTECTED] wrote: global_sigmask is initialized using sigfillset(), so it contains _all_ signals (except SIGSEGV, etc because non-blockable or not safe to block) and there is no need to add signals to global_sigmask it in zend_signal() / zend_signal_register() :) Right you are. Apparently I saw it was doing that but it didn't really register. For extensions IMO we should just let them use zend_signal() as it currently is. Using pcntl_signal() will override handlers previously set by zend_signal() but the signal will remain deferred. I don't see how it would continue to be deferred. Any signal handler registered using zend_signal()/zend_sigaction() is deferred as these functions actually register zend_signal_handler_differ(), which calls the actual handler depending if critical section or not ;) In the case of pcntl_signal(), if it registers a handler for a signal, the signal will be delivered to zend_signal_handler_defer(), which will call zend_signal_handler() if not in critical section, which will call the actual handler registered by pcntl_signal(), pcntl_signal_handler(). Then the user signal handler will be called at the next tick or the next time zend_signal_dispatch() is called. We'd be relying on the fact that pcntl depends on a tick or call to the dispatch function to actually invoke any userspace code. Once registering a signal via pcntl I'm able to jump out of a critical section into pctl_signal_handler when a signal is received. This code might be safe since it doesn't do very much but I don't get real protection. I updated the patches on the wiki: - Added zend_sigaction() - Ported PCNTL to use zend_sigaction() - Fixed handling of SIG_IGN Great. I'll pull these down. -lucas -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
On Tuesday 05 August 2008 08:51:33 Arnaud Le Blanc wrote: Votre message: Greetings! On 8/3/08 9:37 PM, Arnaud LB [EMAIL PROTECTED] wrote: If sigaction is not available Zend Signal Handling will not be enabled, so it will not be enabled on Windows (I assume sigaction is not available on Windows, it is ?). For pthreads and sigprocmask, tsrm_sigmask() can be improved to add support for other APIs. However it seems that only pthread provides that. Apache/APR relies on phtread_sigmask() when available, or sigprocmask() when not. Ahh correct. Wasn't looking at this correctly. As for non pthread sigprocmask sounds like we'll find out if it's problematic in any way. Good to know this is at least what apr is doing. If in the future we have anything in the stack registering a signal handler via zend_signal this will be problematic because SIGG_*_CRITICAL sections will not cover the signals registered post-startup. sigaction() is process-wide, so if a thread registers a signal post-startup (this is already the case, zend_sigs[] are registered at request-startup) the signal will be defered in all threads :) The deferring will basically work in the new model but I'm relying on sigprocmask(SIG_BLOCK/SIG_SETMASK) in the SIGNAL_CRITICAL macro's to protect against jumps during queue maintenance and in zend_signal_handler_unblock when it calls zend_signal_handler_defer. If a post startup signal is registered, the latest version of zend_signal doesn't add the new signal number to the mask, it only sets sa_mask to globals_sigmask. If we add the registered signal to the global_sigmask before setting sa_mask here then I think we would be ok. global_sigmask is initialized using sigfillset(), so it contains _all_ signals (except SIGSEGV, etc because non-blockable or not safe to block) and there is no need to add signals to global_sigmask it in zend_signal() / zend_signal_register() :) I originally did not use an array as I was hoping to dynamically allocate more queue at runtime. I hadn't fully thought it out because there are almost no calls into the code where this would be apropriate. We could maybe do something in zend_signal_handler_unblock but it might be too late there and with this it would not be possible. I don't think I have a problem with this though. Not that it won't happen but I can't imagine a scenario where we get hammered with more signals than we would have slots in ZEND_SIGNAL_QUEUE_SIZE and it wouldn't be safe to ignore them. At least with the signals we're handling out of the box. This could change when we get pcntl working with this. Yes, it may be great to be able to grow the queue when necessary. I'll make a note in the considerations section of the wiki that when we get zend_signal ready to hook up to pctnl we'll reconsider the static allocation. For extensions IMO we should just let them use zend_signal() as it currently is. Using pcntl_signal() will override handlers previously set by zend_signal() but the signal will remain deferred. I was also thinking to implement zend_sigaction(), allowing to know the previously registered signal, and to set some flags when registering signal handlers. -lucas I updated the patches on the wiki: - Added zend_sigaction() - Ported PCNTL to use zend_sigaction() - Fixed handling of SIG_IGN Regards, Arnaud -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
On 8/5/08 3:45 AM, Arnaud Le Blanc [EMAIL PROTECTED] wrote:
In the case of pcntl_signal(), if it registers a handler for a signal, the
signal will be delivered to zend_signal_handler_defer(), which will call
zend_signal_handler() if not in critical section, which will call the actual
handler registered by pcntl_signal(), pcntl_signal_handler(). Then the user
signal handler will be called at the next tick or the next time
zend_signal_dispatch() is called.
Ah well I would have to be running on the latest patch. I just integrated
and it looks good. This did seem a bit weird though:
+++ Zend/zend_hash.c(.../HEAD_signals)(revision 34)
@@ -509,6 +509,9 @@
{
uint nIndex;
Bucket *p;
+TSRMLS_FETCH();
+TSRMLS_FETCH();
+TSRMLS_FETCH();
I took all but one of these out and also have fixed a couple trailing
whitespace/tab issues in my git repo. Just to keep everything consistent
I'll update the patches tomorrow.
Considering our status now it does seem like a good point to ask for a
commit, at least to HEAD. I assume now is also the time for things to be
committed to 5.3 for alpha 2.
-lucas
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
Greetings! On 8/3/08 9:37 PM, Arnaud LB [EMAIL PROTECTED] wrote: If sigaction is not available Zend Signal Handling will not be enabled, so it will not be enabled on Windows (I assume sigaction is not available on Windows, it is ?). For pthreads and sigprocmask, tsrm_sigmask() can be improved to add support for other APIs. However it seems that only pthread provides that. Apache/APR relies on phtread_sigmask() when available, or sigprocmask() when not. Ahh correct. Wasn't looking at this correctly. As for non pthread sigprocmask sounds like we'll find out if it's problematic in any way. Good to know this is at least what apr is doing. If in the future we have anything in the stack registering a signal handler via zend_signal this will be problematic because SIGG_*_CRITICAL sections will not cover the signals registered post-startup. sigaction() is process-wide, so if a thread registers a signal post-startup (this is already the case, zend_sigs[] are registered at request-startup) the signal will be defered in all threads :) The deferring will basically work in the new model but I'm relying on sigprocmask(SIG_BLOCK/SIG_SETMASK) in the SIGNAL_CRITICAL macro's to protect against jumps during queue maintenance and in zend_signal_handler_unblock when it calls zend_signal_handler_defer. If a post startup signal is registered, the latest version of zend_signal doesn't add the new signal number to the mask, it only sets sa_mask to globals_sigmask. If we add the registered signal to the global_sigmask before setting sa_mask here then I think we would be ok. I originally did not use an array as I was hoping to dynamically allocate more queue at runtime. I hadn't fully thought it out because there are almost no calls into the code where this would be apropriate. We could maybe do something in zend_signal_handler_unblock but it might be too late there and with this it would not be possible. I don't think I have a problem with this though. Not that it won't happen but I can't imagine a scenario where we get hammered with more signals than we would have slots in ZEND_SIGNAL_QUEUE_SIZE and it wouldn't be safe to ignore them. At least with the signals we're handling out of the box. This could change when we get pcntl working with this. Yes, it may be great to be able to grow the queue when necessary. I'll make a note in the considerations section of the wiki that when we get zend_signal ready to hook up to pctnl we'll reconsider the static allocation. -lucas -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
Reviewed to my ability and will soon be posting the updated patch for 5_3 and 6 as well as wiki changes. On 8/1/08 6:45 PM, Arnaud Le Blanc [EMAIL PROTECTED] wrote: The ZTS-enabled version of your patch can be found at [1] :) Changes: Zend Signal Handling is now enabled in ZTS builds. Should we be making sure we're not on windows before fully enabling in ZTS builds? This could also affect platforms that do not have PTHREADS available. I assume in the latter case we'll compile but standard sigprocmask will not be be safe enough. SIGALRM in zend_sigs[] has been replaced by a TIMEOUT_SIG macro which is defined to SIGPROF or SIGALRM depending on __CYGWIN__. Nice. As sigmasks are thread-specific it is needed to keep sigmask consistent across all threads. So I just made sigmask a true global (and removed it from the module globals). It is initialized at process startup with sigfillset() (and not modified after that), so it contains all signals (except SIGILL, SIGSEGV, etc). This avoids having to maintain the sigmask each time a signal is registered/added and makes the code simpler (and also saves many sigprocmask() calls). If in the future we have anything in the stack registering a signal handler via zend_signal this will be problematic because SIGG_*_CRITICAL sections will not cover the signals registered post-startup. However the handling of SIG_DFL is not optimal for now as it needs to reset the signal handler to SIG_DFL and call raise(). This is not a problem in non-ZTS but in ZTS we must assume that if a signal must be defered, a signal handler must be registered in all threads for this signal, or the signal must be masked in all threads not having a handler for it. Some changes have been made to use sa_handler or sa_sigaction depending on sa_flags as it is unspecified if they points to a union or not. It seems pretty safe as I haven't seen mention of a system where this is not a union. I thought about making these changes anyway but didn't yet. Thanks. The pstorage global have been added to store the queue, declared as an array of ZEND_SIGNAL_QUEUE_SIZE. This avoids having to allocate/free it and removes the multiple alloc/free for the queue. I originally did not use an array as I was hoping to dynamically allocate more queue at runtime. I hadn't fully thought it out because there are almost no calls into the code where this would be apropriate. We could maybe do something in zend_signal_handler_unblock but it might be too late there and with this it would not be possible. I don't think I have a problem with this though. Not that it won't happen but I can't imagine a scenario where we get hammered with more signals than we would have slots in ZEND_SIGNAL_QUEUE_SIZE and it wouldn't be safe to ignore them. At least with the signals we're handling out of the box. This could change when we get pcntl working with this. To allow that to build I had to add some TSRMLS_FETCH() in zend_alloc.c and zend_hash.c. This is not optimal but the TSRM argument marcos can probably be added to these functions in the future. I'll put this in as is and hopefully someone more familiar with those will be able to review. The tests I made shows that this greatly improves the stability of the ZTS-enabled PHP on non-Windows plateforms, however there are some code which needs to be protected against interruptions (e.g. zend_llist), or to have a wider range protected (zend_hash, zend_alloc). I done that in zend_alloc.c. I will send a version for HEAD later. [1] http://arnaud.lb.s3.amazonaws.com/zend_signal_1217635857.patch I got some build errors when building sapi/cli with and without ZTS. This seems to come from this change to the original diff in zend_signal.c: -zend_closures.c zend_signal.c) +zend_closures.c) I've removed this. I also took out the unused HANDLE_UNBLOCK_INTERRUPTIONS macro. Feel free to add anything to the wiki you think would be pertinent to the RFC. -lucas -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
hi! On Sun, Aug 3, 2008 at 9:34 AM, Lucas Nealan [EMAIL PROTECTED] wrote: Reviewed to my ability and will soon be posting the updated patch for 5_3 and 6 as well as wiki changes. On 8/1/08 6:45 PM, Arnaud Le Blanc [EMAIL PROTECTED] wrote: The ZTS-enabled version of your patch can be found at [1] :) Changes: Zend Signal Handling is now enabled in ZTS builds. Should we be making sure we're not on windows before fully enabling in ZTS builds? This could also affect platforms that do not have PTHREADS available. I assume in the latter case we'll compile but standard sigprocmask will not be be safe enough. We also provide binaries for non ZTS mode, so if the patch is not ready for Windows, it should disable this feature on Windows. Please let us know once you are far enough to envisage a port to windows. We can then try to do it and enable it in our snapshots (once your patch is committed :). Cheers, -- Pierre http://blog.thepimp.net | http://www.libgd.org -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
On 8/3/08 3:00 AM, Pierre Joye [EMAIL PROTECTED] wrote: We also provide binaries for non ZTS mode, so if the patch is not ready for Windows, it should disable this feature on Windows. I was presuming that non-zts, non-cygwin windows will not satisfy the sigaction requirement, thus we wouldn't need to explicitly disable it unless we are doing zts. I could surely be wrong! -lucas -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
Hi Lucas, On Sun, Aug 3, 2008 at 11:06 PM, Lucas Nealan [EMAIL PROTECTED] wrote: On 8/3/08 3:00 AM, Pierre Joye [EMAIL PROTECTED] wrote: We also provide binaries for non ZTS mode, so if the patch is not ready for Windows, it should disable this feature on Windows. I was presuming that non-zts, non-cygwin windows will not satisfy the sigaction requirement, thus we wouldn't need to explicitly disable it unless we are doing zts. I could surely be wrong! I do not know either, only pointed that we release non ZTS windows build as well, just in case :) Cheers, -- Pierre http://blog.thepimp.net | http://www.libgd.org -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
I do not know either, only pointed that we release non ZTS windows build as well, just in case :) So user confusion is not an issue here? Just asking, on account of the amount of user confusion over NTS/ZTS in php-gtk2 history since that became an option... - Steph -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
On Mon, Aug 4, 2008 at 1:33 AM, Steph Fox [EMAIL PROTECTED] wrote: I do not know either, only pointed that we release non ZTS windows build as well, just in case :) So user confusion is not an issue here? It is off topic yes. The only point discussed here is whether the signal handling should be disabled in windows (ZTS and non ZTS) or not until it is ported (if possible/necessary). -- Pierre http://blog.thepimp.net | http://www.libgd.org -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
On Sun, Aug 3, 2008 at 9:34 AM, Lucas Nealan [EMAIL PROTECTED] wrote: Reviewed to my ability and will soon be posting the updated patch for 5_3 and 6 as well as wiki changes. On 8/1/08 6:45 PM, Arnaud Le Blanc [EMAIL PROTECTED] wrote: The ZTS-enabled version of your patch can be found at [1] :) Changes: Zend Signal Handling is now enabled in ZTS builds. Should we be making sure we're not on windows before fully enabling in ZTS builds? This could also affect platforms that do not have PTHREADS available. I assume in the latter case we'll compile but standard sigprocmask will not be be safe enough. If sigaction is not available Zend Signal Handling will not be enabled, so it will not be enabled on Windows (I assume sigaction is not available on Windows, it is ?). For pthreads and sigprocmask, tsrm_sigmask() can be improved to add support for other APIs. However it seems that only pthread provides that. Apache/APR relies on phtread_sigmask() when available, or sigprocmask() when not. SIGALRM in zend_sigs[] has been replaced by a TIMEOUT_SIG macro which is defined to SIGPROF or SIGALRM depending on __CYGWIN__. Nice. As sigmasks are thread-specific it is needed to keep sigmask consistent across all threads. So I just made sigmask a true global (and removed it from the module globals). It is initialized at process startup with sigfillset() (and not modified after that), so it contains all signals (except SIGILL, SIGSEGV, etc). This avoids having to maintain the sigmask each time a signal is registered/added and makes the code simpler (and also saves many sigprocmask() calls). If in the future we have anything in the stack registering a signal handler via zend_signal this will be problematic because SIGG_*_CRITICAL sections will not cover the signals registered post-startup. sigaction() is process-wide, so if a thread registers a signal post-startup (this is already the case, zend_sigs[] are registered at request-startup) the signal will be defered in all threads :) However the handling of SIG_DFL is not optimal for now as it needs to reset the signal handler to SIG_DFL and call raise(). This is not a problem in non-ZTS but in ZTS we must assume that if a signal must be defered, a signal handler must be registered in all threads for this signal, or the signal must be masked in all threads not having a handler for it. Some changes have been made to use sa_handler or sa_sigaction depending on sa_flags as it is unspecified if they points to a union or not. It seems pretty safe as I haven't seen mention of a system where this is not a union. I thought about making these changes anyway but didn't yet. Thanks. The pstorage global have been added to store the queue, declared as an array of ZEND_SIGNAL_QUEUE_SIZE. This avoids having to allocate/free it and removes the multiple alloc/free for the queue. I originally did not use an array as I was hoping to dynamically allocate more queue at runtime. I hadn't fully thought it out because there are almost no calls into the code where this would be apropriate. We could maybe do something in zend_signal_handler_unblock but it might be too late there and with this it would not be possible. I don't think I have a problem with this though. Not that it won't happen but I can't imagine a scenario where we get hammered with more signals than we would have slots in ZEND_SIGNAL_QUEUE_SIZE and it wouldn't be safe to ignore them. At least with the signals we're handling out of the box. This could change when we get pcntl working with this. Yes, it may be great to be able to grow the queue when necessary. To allow that to build I had to add some TSRMLS_FETCH() in zend_alloc.c and zend_hash.c. This is not optimal but the TSRM argument marcos can probably be added to these functions in the future. I'll put this in as is and hopefully someone more familiar with those will be able to review. The tests I made shows that this greatly improves the stability of the ZTS-enabled PHP on non-Windows plateforms, however there are some code which needs to be protected against interruptions (e.g. zend_llist), or to have a wider range protected (zend_hash, zend_alloc). I done that in zend_alloc.c. I will send a version for HEAD later. [1] http://arnaud.lb.s3.amazonaws.com/zend_signal_1217635857.patch I got some build errors when building sapi/cli with and without ZTS. This seems to come from this change to the original diff in zend_signal.c: -zend_closures.c zend_signal.c) +zend_closures.c) Sorry :) I've removed this. I also took out the unused HANDLE_UNBLOCK_INTERRUPTIONS macro. Feel free to add anything to the wiki you think would be pertinent to the RFC. -lucas -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
Hi, On Friday 01 August 2008 05:39:27 Lucas Nealan wrote: I was initially planning to implement ZTS, however the more I learned the harder it became. The first issue being that not every scope implementing the blocking macros has implemented or fetched TSRMLS data. Many places in zend_alloc.c for example fail with the new macro's because of the missing rsrc_id. Also, while this will eventually add some safer ZTS blocking protection it will definitely come at a performance cost instead of the gain we might see in non-zts mode. I wasn't ready to jump in a start changing too much of zend for this to be possible. Most of these are in zend_alloc.c and zend_hash.c. In addition we'd need also need to figure out how manage signal delivery to the right thread. I worked a bit on the ZTS version, this actually fixes many problems with ZTS on non-windows plateforms :) For the delivery to the right thread, there will be no problem as long as the signal is sent to a specific thred (and not to the whole process). setitimer() for example will send the SIGALRM/SIGPROF signal to the calling thread, so the signal will be delivered to the thread which exceeded max_execution_time. For the ts resources however this effectively needs some TSRMLS_FETCH(). As _emalloc() and some others already use TSRMLS_FETCH() I guess it may be possible to just pass the TSRM arguments to those functions. I will send a modified version of your patch tomorrow. Regards, Arnaud -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
Hi! On 7/31/08 11:07 PM, Arnaud Le Blanc [EMAIL PROTECTED] wrote: I worked a bit on the ZTS version, this actually fixes many problems with ZTS on non-windows plateforms :) [...snip...] I will send a modified version of your patch tomorrow. Sounds great, can't wait to see what you've got. I've updated the patch based on some of the feedback received. Please try to incorporate the latest changes into your work. I imagine some of my latest changes may or may not work in ZTS. Maybe this would be easier if we were in a repo ;). Here's a summary of what I've changed: - Fixed failing tests, one was a real problem in the ITIMER signal handling, there was no need to reset SIGPROF to SIG_DFL in zend_unset_timeout. - Moved queue memory allocation to module startup and shutdown, created a new zend_signal_shutdown to facilitate this that is called by zend_shutdown. - Wrapped the deactivate checks in an ini setting, zend.signal_check. The destructor visibility test was failing because we were exiting from within a critical section. This caused the depth counter to be off in deactivate and the test would fail in debug mode because of this output. It's now a ZEND_CORE_WARNING. - Removed SIGG_UNEXPECTED and replaced with Zend's UNEXPECTED macro. A few small todo items related to these changes are: * Update documentation for the zend.signal_check ini * Update documentation to state that ZEND_CORE_WARNING could also be thrown in shutdown in addition to startup. Patch: http://sizzo.org/~screen/patches/php-5.3.0-alarms-0808010408.patch RFC: http://wiki.php.net/rfc/zendsignals -lucas -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
On Friday 01 August 2008 13:27:44 Lucas Nealan wrote: Hi! On 7/31/08 11:07 PM, Arnaud Le Blanc [EMAIL PROTECTED] wrote: I worked a bit on the ZTS version, this actually fixes many problems with ZTS on non-windows plateforms :) [...snip...] I will send a modified version of your patch tomorrow. Sounds great, can't wait to see what you've got. I've updated the patch based on some of the feedback received. Please try to incorporate the latest changes into your work. I imagine some of my latest changes may or may not work in ZTS. Maybe this would be easier if we were in a repo ;). Here's a summary of what I've changed: - Fixed failing tests, one was a real problem in the ITIMER signal handling, there was no need to reset SIGPROF to SIG_DFL in zend_unset_timeout. - Moved queue memory allocation to module startup and shutdown, created a new zend_signal_shutdown to facilitate this that is called by zend_shutdown. - Wrapped the deactivate checks in an ini setting, zend.signal_check. The destructor visibility test was failing because we were exiting from within a critical section. This caused the depth counter to be off in deactivate and the test would fail in debug mode because of this output. It's now a ZEND_CORE_WARNING. - Removed SIGG_UNEXPECTED and replaced with Zend's UNEXPECTED macro. A few small todo items related to these changes are: * Update documentation for the zend.signal_check ini * Update documentation to state that ZEND_CORE_WARNING could also be thrown in shutdown in addition to startup. Patch: http://sizzo.org/~screen/patches/php-5.3.0-alarms-0808010408.patch RFC: http://wiki.php.net/rfc/zendsignals -lucas Hi, The ZTS-enabled version of your patch can be found at [1] :) Changes: Zend Signal Handling is now enabled in ZTS builds. SIGALRM in zend_sigs[] has been replaced by a TIMEOUT_SIG macro which is defined to SIGPROF or SIGALRM depending on __CYGWIN__. As sigmasks are thread-specific it is needed to keep sigmask consistent across all threads. So I just made sigmask a true global (and removed it from the module globals). It is initialized at process startup with sigfillset() (and not modified after that), so it contains all signals (except SIGILL, SIGSEGV, etc). This avoids having to maintain the sigmask each time a signal is registered/added and makes the code simpler (and also saves many sigprocmask() calls). A tsrm_sigmask() function has been added to call thread-specific routines instead of calling sigprocmask() directly (e.g. pthread_sigmask for pthreads). I added it in TSRM as it is thread-related and TSRM already checks for relevant libraries. As sigaction() is process-wide, a thread can receives a signal even if it has not registered the signal itself. In this case the thread will have to find the original signal handler registered for the signal, which will just not work if it uses sigaction to find it. So an other true global as been added, global_orig_handlers, initialized at process startup with all registered signals. It is then just memcpy()ed into the handlers module global in zend_signal_activate() so that the threads can modify their local copy when registering signals. However the handling of SIG_DFL is not optimal for now as it needs to reset the signal handler to SIG_DFL and call raise(). This is not a problem in non-ZTS but in ZTS we must assume that if a signal must be defered, a signal handler must be registered in all threads for this signal, or the signal must be masked in all threads not having a handler for it. Some changes have been made to use sa_handler or sa_sigaction depending on sa_flags as it is unspecified if they points to a union or not. The pstorage global have been added to store the queue, declared as an array of ZEND_SIGNAL_QUEUE_SIZE. This avoids having to allocate/free it and removes the multiple alloc/free for the queue. To allow that to build I had to add some TSRMLS_FETCH() in zend_alloc.c and zend_hash.c. This is not optimal but the TSRM argument marcos can probably be added to these functions in the future. The tests I made shows that this greatly improves the stability of the ZTS-enabled PHP on non-Windows plateforms, however there are some code which needs to be protected against interruptions (e.g. zend_llist), or to have a wider range protected (zend_hash, zend_alloc). I done that in zend_alloc.c. I will send a version for HEAD later. [1] http://arnaud.lb.s3.amazonaws.com/zend_signal_1217635857.patch Regards, Arnaud -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
On 8/1/08 6:45 PM, Arnaud Le Blanc [EMAIL PROTECTED] wrote: Hi, The ZTS-enabled version of your patch can be found at [1] :) Thanks Arnaud. I've gone ahead and made the HEAD patch already, wasn't as bad as I thought it might be. I'll review this hopefully by tomorrow and then pull into my local git repo. After this I can go ahead and merge over to my local 6.0 branch and post both patches to the wiki. -lucas -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
On 7/30/08 6:11 AM, Lukas Kahwe Smith [EMAIL PROTECTED] wrote: Not so happy that it was not possible to get this committed over the weekend. Johannes did a quick review and it seems like it has enough support from people and is low risk enough to get committed now. Lets hope no extension stumbled over this one .. @Dmitry: Can you get this applied today? On 7/30/08 9:37 AM, Dmitry Stogov [EMAIL PROTECTED] wrote: I think it's not a good idea to commit it in this state. Correct me, if I'm wrong. I've done just a very quick review. Thanks. Dmitry. Sorry about the delay getting some of these things worked on over the weekend. I had to recover from OSCON and didn't get a chance to look at things until back in the office. Seems as though I've been given a stay and can land in Alpha 2 once some more work is done. Thanks for this. In response to Dmitry's feedback and some others: On 7/30/08 9:37 AM, Dmitry Stogov [EMAIL PROTECTED] wrote: I see several issues with the patch 1) It assumes that web server (and webserver extensions) won't setup any signal handlers after PHP startup. This assumption may be wrong. On 7/30/08 9:50 AM, Rasmus Lerdorf [EMAIL PROTECTED] wrote: It may be. But there is really no way around that. That's why we talked about having an optional request shutdown check that would tell you if something hi-jacked one of the signals so at least you know that this is happening. I have to go with what Rasmus said here, theres not much we can do. It seems pretty unlikely since the new signal handler would have to be installed via a SAPI callback. I'm going to go ahead and modify the check in zend_signal_deactivate to be ini configurable and throw a E_CORE_WARNING instead of being enabled via ZEND_DEBUG. This should make it easier to troubleshoot. On 7/30/08 9:37 AM, Dmitry Stogov [EMAIL PROTECTED] wrote: 2) It is incompatible with ext/pcntl On 7/30/08 11:15 AM, Arnaud Le Blanc [EMAIL PROTECTED] wrote: If zend_signal() is improved (has Lucas planed to do) to allow extensions to use it, then pcntl can do so and become compatible with that. I made some notes about this compatibility in the RFC as using pcntl with this won't break anything, we'll just potentially lose some protections. I would like to see both working together but for the initial version I'm not convinced this should be a requirement. 3) It breaks 3 tests (in debug mode) tests/classes/destructor_visibility_001.phpt tests/func/005a.phpt I think the signals may not be setted to SIG_DFL here, as zend_set_timeout() will not always re-install the signal handlers (OnUpdateTimeout() calls zend_unset_timeout() and then zend_set_timeout() with reset_signals=0). ext/standard/tests/general_functions/phpinfo.phpt This is because Zend Signal Handling = %d is missing from the expected result ;) I was pretty sure I had run the test but apparently not. I'll take a look at these and fix right away. Thanks for the pointers Arnaud. We may also want develop some basic tests that we can run against the new functionality. Seems tricky though so I haven't really thought about this much yet. 4) I would also see the patch for HEAD (to commit them together) This does need to happen but I'm unsure whether committing together is benificial. PHP6 isn't in my future yet so I haven't had an opportunity to work with it. I'll happily port this to 6 but it will take some time before I can do that. If it's generally believed to be a condition for inclusion into 5.3, and unless someone steps in to do the port right away, the whole shebang will need to be scheduled into the next 5.x release. What is the general opinion about this? Thanks for the feedback! -lucas -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
On 7/30/08 11:54 AM, Stanisla Malyshev [EMAIL PROTECTED] wrote: I have a couple of questions about the patch: 1. Why allocate fixed-size buffer via individual malloc's on each activate and free it on each deactivate? Won't it be better to just allocate it once and use it? I'll take a look at this. I can't think of a reason not to avoid the alloc's per request. 2. Why define own SIG_UNEXPECTED - we already have UNEXPECTED macro in the engine? Good question. Trying to remember Ok yeah, it's not coming to me. Unless it comes randomly into my head I'll switch this over. 3. I understand that ZEND_SIGNALS is disabled whenever ZTS is on. However, in the code I see parts where under ifdef ZEND_SIGNALS there's still checks for ifdef ZTS. Why is that? Is it planned to be implemented on some stage? I was initially planning to implement ZTS, however the more I learned the harder it became. The first issue being that not every scope implementing the blocking macros has implemented or fetched TSRMLS data. Many places in zend_alloc.c for example fail with the new macro's because of the missing rsrc_id. Also, while this will eventually add some safer ZTS blocking protection it will definitely come at a performance cost instead of the gain we might see in non-zts mode. I wasn't ready to jump in a start changing too much of zend for this to be possible. Most of these are in zend_alloc.c and zend_hash.c. In addition we'd need also need to figure out how manage signal delivery to the right thread. This wasn't something we really needed so I put it off. Instead of yanking all the ZTS references out of the code I left them in for future use. It seems that Pierre was interested in working on support under windows and maybe someone will want to help with non-windws ZTS as well. 4. Why we try to handle SIGTERM and SIGQUIT - aren't those supposed to kill the process anyway? This depends on what the SAPI is doing. In Apache 1.x SIGTERM, as well as SIGINT, will both trigger a full php shutdown via Apache's handler, which if long jumped into from within a critical section will be very likely to segfault. SIGQUIT is usually set to SIG_DFL by Apache 1.x but there's no guarantee what this will be in any other SAPI so the safest route seems to be to include these, I could be wrong. The real risk is that pressing CTRL-C or issuing a kill might cause an actual exit or coredump to be delayed, or not happen at all, if we were truly blocked or stuck within a critical section. Thanks for the useful feedback! -lucas -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
On 30.07.2008, at 01:58, Lucas Nealan wrote: I've updated the patch for Zend Signal Handling, the latest version is available on the wiki rfc page: http://wiki.php.net/rfc/zendsignals The update solves the reentrance issue with using the a zend linked list in the default signal handler. I've also added a debug only check, at least for now, that will output a debug string during shutdown if for some reason any of the handlers installed during startup are no longer present. I've added a discoveries section describing other features that may cause issue with Zend Signals or should be incorporated. These include SIGCHILD handling and pctl_signal. Please read on the wiki for the complete details. Full list of changes: - Replaced zend ll queue with a pre-allocated internal queue (thx pcntl) - Added shutdown check for replaced signal handlers - Added errno protection to the existing sigchld_handler in main.c - Zend Signal handling is now enabled by default if sigaction is present during a non-zts build. - Made the patch more TSRMLS correct even though ZTS won't work - Critical section in zend_signal_handler_unblock via sigprocmask for queue mgmt and so it's zend_signal_handler_defer call will be afforded the same signal queuing as when called by the kernel I've limitedly tested these changes and everything seems normal in gdb. Unless I hear otherwise I believe this to be ready to commit. Not so happy that it was not possible to get this committed over the weekend. Johannes did a quick review and it seems like it has enough support from people and is low risk enough to get committed now. Lets hope no extension stumbled over this one .. @Dmitry: Can you get this applied today? regards, Lukas Kahwe Smith [EMAIL PROTECTED] -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
I'll do quick review in an hour and then I'll probably commit it. Thanks. Dmitry. Lukas Kahwe Smith wrote: On 30.07.2008, at 01:58, Lucas Nealan wrote: I've updated the patch for Zend Signal Handling, the latest version is available on the wiki rfc page: http://wiki.php.net/rfc/zendsignals The update solves the reentrance issue with using the a zend linked list in the default signal handler. I've also added a debug only check, at least for now, that will output a debug string during shutdown if for some reason any of the handlers installed during startup are no longer present. I've added a discoveries section describing other features that may cause issue with Zend Signals or should be incorporated. These include SIGCHILD handling and pctl_signal. Please read on the wiki for the complete details. Full list of changes: - Replaced zend ll queue with a pre-allocated internal queue (thx pcntl) - Added shutdown check for replaced signal handlers - Added errno protection to the existing sigchld_handler in main.c - Zend Signal handling is now enabled by default if sigaction is present during a non-zts build. - Made the patch more TSRMLS correct even though ZTS won't work - Critical section in zend_signal_handler_unblock via sigprocmask for queue mgmt and so it's zend_signal_handler_defer call will be afforded the same signal queuing as when called by the kernel I've limitedly tested these changes and everything seems normal in gdb. Unless I hear otherwise I believe this to be ready to commit. Not so happy that it was not possible to get this committed over the weekend. Johannes did a quick review and it seems like it has enough support from people and is low risk enough to get committed now. Lets hope no extension stumbled over this one .. @Dmitry: Can you get this applied today? regards, Lukas Kahwe Smith [EMAIL PROTECTED] -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
I see several issues with the patch 1) It assumes that web server (and webserver extensions) won't setup any signal handlers after PHP startup. This assumption may be wrong. 2) It is incompatible with ext/pcntl 3) It breaks 3 tests (in debug mode) tests/classes/destructor_visibility_001.phpt tests/func/005a.phpt ext/standard/tests/general_functions/phpinfo.phpt 4) I would also see the patch for HEAD (to commit them together) I think it's not a good idea to commit it in this state. Correct me, if I'm wrong. I've done just a very quick review. Thanks. Dmitry. Lukas Kahwe Smith wrote: On 30.07.2008, at 01:58, Lucas Nealan wrote: I've updated the patch for Zend Signal Handling, the latest version is available on the wiki rfc page: http://wiki.php.net/rfc/zendsignals The update solves the reentrance issue with using the a zend linked list in the default signal handler. I've also added a debug only check, at least for now, that will output a debug string during shutdown if for some reason any of the handlers installed during startup are no longer present. I've added a discoveries section describing other features that may cause issue with Zend Signals or should be incorporated. These include SIGCHILD handling and pctl_signal. Please read on the wiki for the complete details. Full list of changes: - Replaced zend ll queue with a pre-allocated internal queue (thx pcntl) - Added shutdown check for replaced signal handlers - Added errno protection to the existing sigchld_handler in main.c - Zend Signal handling is now enabled by default if sigaction is present during a non-zts build. - Made the patch more TSRMLS correct even though ZTS won't work - Critical section in zend_signal_handler_unblock via sigprocmask for queue mgmt and so it's zend_signal_handler_defer call will be afforded the same signal queuing as when called by the kernel I've limitedly tested these changes and everything seems normal in gdb. Unless I hear otherwise I believe this to be ready to commit. Not so happy that it was not possible to get this committed over the weekend. Johannes did a quick review and it seems like it has enough support from people and is low risk enough to get committed now. Lets hope no extension stumbled over this one .. @Dmitry: Can you get this applied today? regards, Lukas Kahwe Smith [EMAIL PROTECTED] -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
RE: [PHP-DEV] [RFC] Zend Signal Handling
I would need to look into this in more detail but I am not sure sigaction is supported. In any case, these APIs on Windows tend to be more for POSIX compliance and not the Windows way so I am not sure there'd be a lot of value in supporting it. In any case, you should probably still take a look but keep this in mind when you're looking at MSDN :) Andi -Original Message- From: Pierre Joye [mailto:[EMAIL PROTECTED] Sent: Thursday, July 24, 2008 12:27 AM To: Stas Malyshev Cc: Antony Dovgal; Scott MacVicar; Lucas Nealan; internals@lists.php.net; Andi Gutmans Subject: Re: [PHP-DEV] [RFC] Zend Signal Handling On Wed, Jul 23, 2008 at 7:54 PM, Stanislav Malyshev [EMAIL PROTECTED] wrote: Hi! Do we really need this option? Is someone going to disable it and why? I see only reason to disable it if one has some weird system where sigaction is either absent or doesn't work as it should. Not that I know of any, but Unix variants are full of surprises. I'd keep it enabled by default, unless we are on OS that doesn't have sigaction (e.g. Windows or some extremely weird Unix) or in ZTS, of course. Windows has signal (and SigAction) support. Obviously (sigh) not using the same API but it is possible to achieve the same behaviors using the windows API. For example, there is already exception for special cases like SIGALRM, windows API also uses a timer (CreateWaitableTimer co). Once the code is in cvs, I can give it a try to port zend_signal to windows (not before alpha1 but before alpha2 :) Cheers, -- Pierre http://blog.thepimp.net | http://www.libgd.org -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
Dmitry Stogov wrote: I see several issues with the patch 1) It assumes that web server (and webserver extensions) won't setup any signal handlers after PHP startup. This assumption may be wrong. It may be. But there is really no way around that. That's why we talked about having an optional request shutdown check that would tell you if something hi-jacked one of the signals so at least you know that this is happening. 2) It is incompatible with ext/pcntl 3) It breaks 3 tests (in debug mode) tests/classes/destructor_visibility_001.phpt tests/func/005a.phpt ext/standard/tests/general_functions/phpinfo.phpt 4) I would also see the patch for HEAD (to commit them together) I think it's not a good idea to commit it in this state. Correct me, if I'm wrong. I've done just a very quick review. It is much better than what we have now. We currently have nothing that provides any sort of critical section protection from signals. This patch at least gives us this for most cases. The exceptions being weird extensions that set their own signal handlers and pcntl stuff that tries to fiddle with them directly. But in both cases I would argue that you get what you deserve if you are doing that. -Rasmus -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
Hi, On Wednesday 30 July 2008 18:37:26 Dmitry Stogov wrote: I see several issues with the patch 1) It assumes that web server (and webserver extensions) won't setup any signal handlers after PHP startup. This assumption may be wrong. 2) It is incompatible with ext/pcntl If zend_signal() is improved (has Lucas planed to do) to allow extensions to use it, then pcntl can do so and become compatible with that. 3) It breaks 3 tests (in debug mode) tests/classes/destructor_visibility_001.phpt tests/func/005a.phpt It seems that this chunk is responsible of that (in zend_unset_timeout): @@ -1411,8 +1413,14 @@ #ifdef __CYGWIN__ setitimer(ITIMER_REAL, no_timeout, NULL); +# ifdef ZEND_SIGNALS + zend_signal(SIGALRM, SIG_DFL); +# endif #else setitimer(ITIMER_PROF, no_timeout, NULL); +# ifdef ZEND_SIGNALS + zend_signal(SIGPROF, SIG_DFL); +# endif #endif } # endif I think the signals may not be setted to SIG_DFL here, as zend_set_timeout() will not always re-install the signal handlers (OnUpdateTimeout() calls zend_unset_timeout() and then zend_set_timeout() with reset_signals=0). ext/standard/tests/general_functions/phpinfo.phpt This is because Zend Signal Handling = %d is missing from the expected result ;) 4) I would also see the patch for HEAD (to commit them together) I think it's not a good idea to commit it in this state. Correct me, if I'm wrong. I've done just a very quick review. Thanks. Dmitry. Lukas Kahwe Smith wrote: On 30.07.2008, at 01:58, Lucas Nealan wrote: I've updated the patch for Zend Signal Handling, the latest version is available on the wiki rfc page: http://wiki.php.net/rfc/zendsignals The update solves the reentrance issue with using the a zend linked list in the default signal handler. I've also added a debug only check, at least for now, that will output a debug string during shutdown if for some reason any of the handlers installed during startup are no longer present. I've added a discoveries section describing other features that may cause issue with Zend Signals or should be incorporated. These include SIGCHILD handling and pctl_signal. Please read on the wiki for the complete details. Full list of changes: - Replaced zend ll queue with a pre-allocated internal queue (thx pcntl) - Added shutdown check for replaced signal handlers - Added errno protection to the existing sigchld_handler in main.c - Zend Signal handling is now enabled by default if sigaction is present during a non-zts build. - Made the patch more TSRMLS correct even though ZTS won't work - Critical section in zend_signal_handler_unblock via sigprocmask for queue mgmt and so it's zend_signal_handler_defer call will be afforded the same signal queuing as when called by the kernel I've limitedly tested these changes and everything seems normal in gdb. Unless I hear otherwise I believe this to be ready to commit. Not so happy that it was not possible to get this committed over the weekend. Johannes did a quick review and it seems like it has enough support from people and is low risk enough to get committed now. Lets hope no extension stumbled over this one .. @Dmitry: Can you get this applied today? regards, Lukas Kahwe Smith [EMAIL PROTECTED] -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
Arnaud Le Blanc kirjoitti: Hi, On Wednesday 30 July 2008 18:37:26 Dmitry Stogov wrote: I see several issues with the patch 1) It assumes that web server (and webserver extensions) won't setup any signal handlers after PHP startup. This assumption may be wrong. 2) It is incompatible with ext/pcntl If zend_signal() is improved (has Lucas planed to do) to allow extensions to use it, then pcntl can do so and become compatible with that. Since this seems related, maybe someone ought to check this out too: http://bugs.php.net/16820 A bit old bug but has some relation to part below. --Jani 3) It breaks 3 tests (in debug mode) tests/classes/destructor_visibility_001.phpt tests/func/005a.phpt It seems that this chunk is responsible of that (in zend_unset_timeout): @@ -1411,8 +1413,14 @@ #ifdef __CYGWIN__ setitimer(ITIMER_REAL, no_timeout, NULL); +# ifdef ZEND_SIGNALS + zend_signal(SIGALRM, SIG_DFL); +# endif #else setitimer(ITIMER_PROF, no_timeout, NULL); +# ifdef ZEND_SIGNALS + zend_signal(SIGPROF, SIG_DFL); +# endif #endif } # endif I think the signals may not be setted to SIG_DFL here, as zend_set_timeout() will not always re-install the signal handlers (OnUpdateTimeout() calls zend_unset_timeout() and then zend_set_timeout() with reset_signals=0). ext/standard/tests/general_functions/phpinfo.phpt This is because Zend Signal Handling = %d is missing from the expected result ;) 4) I would also see the patch for HEAD (to commit them together) I think it's not a good idea to commit it in this state. Correct me, if I'm wrong. I've done just a very quick review. Thanks. Dmitry. Lukas Kahwe Smith wrote: On 30.07.2008, at 01:58, Lucas Nealan wrote: I've updated the patch for Zend Signal Handling, the latest version is available on the wiki rfc page: http://wiki.php.net/rfc/zendsignals The update solves the reentrance issue with using the a zend linked list in the default signal handler. I've also added a debug only check, at least for now, that will output a debug string during shutdown if for some reason any of the handlers installed during startup are no longer present. I've added a discoveries section describing other features that may cause issue with Zend Signals or should be incorporated. These include SIGCHILD handling and pctl_signal. Please read on the wiki for the complete details. Full list of changes: - Replaced zend ll queue with a pre-allocated internal queue (thx pcntl) - Added shutdown check for replaced signal handlers - Added errno protection to the existing sigchld_handler in main.c - Zend Signal handling is now enabled by default if sigaction is present during a non-zts build. - Made the patch more TSRMLS correct even though ZTS won't work - Critical section in zend_signal_handler_unblock via sigprocmask for queue mgmt and so it's zend_signal_handler_defer call will be afforded the same signal queuing as when called by the kernel I've limitedly tested these changes and everything seems normal in gdb. Unless I hear otherwise I believe this to be ready to commit. Not so happy that it was not possible to get this committed over the weekend. Johannes did a quick review and it seems like it has enough support from people and is low risk enough to get committed now. Lets hope no extension stumbled over this one .. @Dmitry: Can you get this applied today? regards, Lukas Kahwe Smith [EMAIL PROTECTED] -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
Hi! I have a couple of questions about the patch: 1. Why allocate fixed-size buffer via individual malloc's on each activate and free it on each deactivate? Won't it be better to just allocate it once and use it? 2. Why define own SIG_UNEXPECTED - we already have UNEXPECTED macro in the engine? 3. I understand that ZEND_SIGNALS is disabled whenever ZTS is on. However, in the code I see parts where under ifdef ZEND_SIGNALS there's still checks for ifdef ZTS. Why is that? Is it planned to be implemented on some stage? 4. Why we try to handle SIGTERM and SIGQUIT - aren't those supposed to kill the process anyway? -- Stanislav Malyshev, Zend Software Architect [EMAIL PROTECTED] http://www.zend.com/ (408)253-8829 MSN: [EMAIL PROTECTED] -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
On Wednesday 30 July 2008 20:46:13 Jani Taskinen wrote: Arnaud Le Blanc kirjoitti: Hi, On Wednesday 30 July 2008 18:37:26 Dmitry Stogov wrote: I see several issues with the patch 1) It assumes that web server (and webserver extensions) won't setup any signal handlers after PHP startup. This assumption may be wrong. 2) It is incompatible with ext/pcntl If zend_signal() is improved (has Lucas planed to do) to allow extensions to use it, then pcntl can do so and become compatible with that. Since this seems related, maybe someone ought to check this out too: http://bugs.php.net/16820 A bit old bug but has some relation to part below. Hi, This new signal handling may fix this sort of things if enabled in ZTS. I think this patch may include support for ZTS too (I'm investigating how it can). Regards, Arnaud --Jani 3) It breaks 3 tests (in debug mode) tests/classes/destructor_visibility_001.phpt tests/func/005a.phpt It seems that this chunk is responsible of that (in zend_unset_timeout): @@ -1411,8 +1413,14 @@ #ifdef __CYGWIN__ setitimer(ITIMER_REAL, no_timeout, NULL); +# ifdef ZEND_SIGNALS + zend_signal(SIGALRM, SIG_DFL); +# endif #else setitimer(ITIMER_PROF, no_timeout, NULL); +# ifdef ZEND_SIGNALS + zend_signal(SIGPROF, SIG_DFL); +# endif #endif } # endif I think the signals may not be setted to SIG_DFL here, as zend_set_timeout() will not always re-install the signal handlers (OnUpdateTimeout() calls zend_unset_timeout() and then zend_set_timeout() with reset_signals=0). ext/standard/tests/general_functions/phpinfo.phpt This is because Zend Signal Handling = %d is missing from the expected result ;) 4) I would also see the patch for HEAD (to commit them together) I think it's not a good idea to commit it in this state. Correct me, if I'm wrong. I've done just a very quick review. Thanks. Dmitry. Lukas Kahwe Smith wrote: On 30.07.2008, at 01:58, Lucas Nealan wrote: I've updated the patch for Zend Signal Handling, the latest version is available on the wiki rfc page: http://wiki.php.net/rfc/zendsignals The update solves the reentrance issue with using the a zend linked list in the default signal handler. I've also added a debug only check, at least for now, that will output a debug string during shutdown if for some reason any of the handlers installed during startup are no longer present. I've added a discoveries section describing other features that may cause issue with Zend Signals or should be incorporated. These include SIGCHILD handling and pctl_signal. Please read on the wiki for the complete details. Full list of changes: - Replaced zend ll queue with a pre-allocated internal queue (thx pcntl) - Added shutdown check for replaced signal handlers - Added errno protection to the existing sigchld_handler in main.c - Zend Signal handling is now enabled by default if sigaction is present during a non-zts build. - Made the patch more TSRMLS correct even though ZTS won't work - Critical section in zend_signal_handler_unblock via sigprocmask for queue mgmt and so it's zend_signal_handler_defer call will be afforded the same signal queuing as when called by the kernel I've limitedly tested these changes and everything seems normal in gdb. Unless I hear otherwise I believe this to be ready to commit. Not so happy that it was not possible to get this committed over the weekend. Johannes did a quick review and it seems like it has enough support from people and is low risk enough to get committed now. Lets hope no extension stumbled over this one .. @Dmitry: Can you get this applied today? regards, Lukas Kahwe Smith [EMAIL PROTECTED] -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
I've updated the patch for Zend Signal Handling, the latest version is available on the wiki rfc page: http://wiki.php.net/rfc/zendsignals The update solves the reentrance issue with using the a zend linked list in the default signal handler. I've also added a debug only check, at least for now, that will output a debug string during shutdown if for some reason any of the handlers installed during startup are no longer present. I've added a discoveries section describing other features that may cause issue with Zend Signals or should be incorporated. These include SIGCHILD handling and pctl_signal. Please read on the wiki for the complete details. Full list of changes: - Replaced zend ll queue with a pre-allocated internal queue (thx pcntl) - Added shutdown check for replaced signal handlers - Added errno protection to the existing sigchld_handler in main.c - Zend Signal handling is now enabled by default if sigaction is present during a non-zts build. - Made the patch more TSRMLS correct even though ZTS won't work - Critical section in zend_signal_handler_unblock via sigprocmask for queue mgmt and so it's zend_signal_handler_defer call will be afforded the same signal queuing as when called by the kernel I've limitedly tested these changes and everything seems normal in gdb. Unless I hear otherwise I believe this to be ready to commit. -lucas -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
I was waiting after Rasmus said he wanted to compare to the internal Signals code they have at Yahoo before asking againd about inclusion. Gopal is familiar with the Yahoo code as well and we're planning to get together tomorrow and to review and make sure there aren't any critial oversights on this implementation. If the timeline allows 5.3 would be great but we may want to wait until Friday to commit. Discussed this a bit with Rasmus and Gopal yesterday. There are a couple scenarios that do need to be addressed however since having this is still an improvement over not, the thought was to commit and apply fixes shortly either in 5.3 or whatever comes afterwards. The biggest concern is the usage of a linked list in the default handler. If the handler is invoked within an alloc, appending to the ll will cause a recursive alloc. I added queuing of the deferred signals recently after originally using a static table but it seems the latter will be more stable and queuing is probably not so important. I can revert back to using a static table quickly, either in the initial patch, depends on who commits how fast, or as a new patch against 5.3 after the original is committed since I have no karma. Another thought was to add a check in rshutdown to determine if an extension had replaced the default zend handler that is setup in rinit. This could be useful for debugging and I assume we would want to trigger a E_CORE_WARNING with hopefully enough information to know which extension it was. Probably don't need this right away though. -lucas -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
On Wed, Jul 23, 2008 at 7:54 PM, Stanislav Malyshev [EMAIL PROTECTED] wrote: Hi! Do we really need this option? Is someone going to disable it and why? I see only reason to disable it if one has some weird system where sigaction is either absent or doesn't work as it should. Not that I know of any, but Unix variants are full of surprises. I'd keep it enabled by default, unless we are on OS that doesn't have sigaction (e.g. Windows or some extremely weird Unix) or in ZTS, of course. Windows has signal (and SigAction) support. Obviously (sigh) not using the same API but it is possible to achieve the same behaviors using the windows API. For example, there is already exception for special cases like SIGALRM, windows API also uses a timer (CreateWaitableTimer co). Once the code is in cvs, I can give it a try to port zend_signal to windows (not before alpha1 but before alpha2 :) Cheers, -- Pierre http://blog.thepimp.net | http://www.libgd.org -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
On 06.07.2008, at 20:56, Lucas Nealan wrote: Hi Internals, I am proposing the following RFC to improve signal handling in the Zend Engine: http://wiki.php.net/rfc/zendsignals The purpose of zend internal deferred signal handling is to improve the stability of PHP and extensions when running under any forking SAPI. Additionally heavy users of APC or other bytecode caches under Apache 1.x may also see a performance improvement. Please see follow the link above to the complete RFC for details. I look forward to hearing feedback. Whats going on here. How does this all affect windows? Is this planned to go into 5.3? regards, Lukas Kahwe Smith [EMAIL PROTECTED] -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
On 06.07.2008 22:56, Lucas Nealan wrote: Hi Internals, I am proposing the following RFC to improve signal handling in the Zend Engine: http://wiki.php.net/rfc/zendsignals The RFC looks really nice, but we need to make a decision on it really fast, since 5_3 feature freeze is set for tomorrow. I believe this can should go in 5_3, any objections? -- Wbr, Antony Dovgal -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
Antony Dovgal wrote: On 06.07.2008 22:56, Lucas Nealan wrote: Hi Internals, I am proposing the following RFC to improve signal handling in the Zend Engine: http://wiki.php.net/rfc/zendsignals The RFC looks really nice, but we need to make a decision on it really fast, since 5_3 feature freeze is set for tomorrow. I believe this can should go in 5_3, any objections? Enable it by default and change -–enable-zend-signals to --disable-zend-signals Would be nice to get some sort of ZTS implementation in there, i'm less concerned about Windows it can be added at some other point. If it doesn't work out we can just disable it by default for the 5.3 release. Scott -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
On 23.07.2008 15:42, Scott MacVicar wrote: http://wiki.php.net/rfc/zendsignals The RFC looks really nice, but we need to make a decision on it really fast, since 5_3 feature freeze is set for tomorrow. I believe this can should go in 5_3, any objections? Enable it by default and change -–enable-zend-signals to --disable-zend-signals Do we really need this option? Is someone going to disable it and why? -- Wbr, Antony Dovgal -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
Antony Dovgal wrote: On 23.07.2008 15:42, Scott MacVicar wrote: http://wiki.php.net/rfc/zendsignals The RFC looks really nice, but we need to make a decision on it really fast, since 5_3 feature freeze is set for tomorrow. I believe this can should go in 5_3, any objections? Enable it by default and change -–enable-zend-signals to --disable-zend-signals Do we really need this option? Is someone going to disable it and why? The defines need to be there for anyone who doesn't have sigaction available, I guess I'm not bothered about having the option but if it is there it shouldn't be an explicit --enable Scott -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
On 23.07.2008 16:08, Scott MacVicar wrote: Do we really need this option? Is someone going to disable it and why? The defines need to be there for anyone who doesn't have sigaction available PHP_CHECK_FUNC(sigaction) in configure.in should be enough for that. -- Wbr, Antony Dovgal -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
RE: [PHP-DEV] [RFC] Zend Signal Handling
I want to give this a last consideration (by tomorrow). Main things are: - Checking into sigaction() support on various UNIX flavors. I am not sure that all exotic systems have it available and we want to be able to run on those (e.g. older IRIX, AIX, HP-UX versions). - Want to see what to do on Windows if anything. I don't believe we really covered this previously accept for our timeout thread so I think it is a non issue. I would prefer this patch to be in PHP 5.3 but I have a feeling we should keep the patch as-is and enable this to be configured out (don't care if it's implicit and not explicit). Andi -Original Message- From: Antony Dovgal [mailto:[EMAIL PROTECTED] Sent: Wednesday, July 23, 2008 5:20 AM To: Scott MacVicar Cc: Lucas Nealan; internals@lists.php.net Subject: Re: [PHP-DEV] [RFC] Zend Signal Handling On 23.07.2008 16:08, Scott MacVicar wrote: Do we really need this option? Is someone going to disable it and why? The defines need to be there for anyone who doesn't have sigaction available PHP_CHECK_FUNC(sigaction) in configure.in should be enough for that. -- Wbr, Antony Dovgal -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
Hi! Do we really need this option? Is someone going to disable it and why? I see only reason to disable it if one has some weird system where sigaction is either absent or doesn't work as it should. Not that I know of any, but Unix variants are full of surprises. I'd keep it enabled by default, unless we are on OS that doesn't have sigaction (e.g. Windows or some extremely weird Unix) or in ZTS, of course. -- Stanislav Malyshev, Zend Software Architect [EMAIL PROTECTED] http://www.zend.com/ (408)253-8829 MSN: [EMAIL PROTECTED] -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
I was waiting after Rasmus said he wanted to compare to the internal Signals code they have at Yahoo before asking againd about inclusion. Gopal is familiar with the Yahoo code as well and we're planning to get together tomorrow and to review and make sure there aren't any critial oversights on this implementation. If the timeline allows 5.3 would be great but we may want to wait until Friday to commit. It makes sense to me to enable by default on systems that have sigaction and disable on those without. This happens curretly in the patch if it's enabled and not present. I did check some discussions from other projects where this has come up and of was found to be available on almost all systems a number of years ago. I can dig this up and add to the rfc soon. -lucas (mobile) On Jul 23, 2008, at 10:55, Stanislav Malyshev [EMAIL PROTECTED] wrote: Hi! Do we really need this option? Is someone going to disable it and why? I see only reason to disable it if one has some weird system where sigaction is either absent or doesn't work as it should. Not that I know of any, but Unix variants are full of surprises. I'd keep it enabled by default, unless we are on OS that doesn't have sigaction (e.g. Windows or some extremely weird Unix) or in ZTS, of course. -- Stanislav Malyshev, Zend Software Architect [EMAIL PROTECTED] http://www.zend.com/ (408)253-8829 MSN: [EMAIL PROTECTED] -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
Hi Johannes,
thanks for the patch, next to platform specific stuff I'm wondering
whether the shutdown order is right:
[..snip..]
Destructors can be PHP code, as can some ob callback so I think the order
should be changed.
Yes, great catch, the order is incorrect. We use a different order
internally that didn¹t translate properly when I merged to php 5.3. My goal
is to have the unset be as high as possible in the shutdown after all
possible userspace code is executed as well as response is sent but before
any real executor teardown starts happening. I¹ve updated the patch attached
to the rfc which moves the timeout unset in php_request_shutdown from #2,
after php_call_shutdown_functions to #5, after sapi_send_headers. Does this
seem better?
/* 4. Send the set HTTP headers (note: This must be done AFTER
php_end_ob_buffers() !!) */
zend_try {
sapi_send_headers(TSRMLS_C);
} zend_end_try();
/* 5. Reset max_execution_time (no longer executing php code after response
sent) */
zend_try {
zend_unset_timeout(TSRMLS_C);
} zend_end_try();
/* 6. Call all extensions RSHUTDOWN functions */
if (PG(modules_activated)) {
zend_deactivate_modules(TSRMLS_C);
php_free_shutdown_functions(TSRMLS_C);
}
-lucas
Re: [PHP-DEV] [RFC] Zend Signal Handling
I like the idea, and I think we don't need --enable-signals options. BTW I'm not sure about committing it into 5.3. It's a question to RM(s). Thanks. Dmitry. Lucas Nealan wrote: Hi Internals, I am proposing the following RFC to improve signal handling in the Zend Engine: http://wiki.php.net/rfc/zendsignals The purpose of zend internal deferred signal handling is to improve the stability of PHP and extensions when running under any forking SAPI. Additionally heavy users of APC or other bytecode caches under Apache 1.x may also see a performance improvement. Please see follow the link above to the complete RFC for details. I look forward to hearing feedback. -lucas -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
Hi Dmitry, I like the idea, and I think we don't need --enable-signals options. I would like to have this enabled by default and this would be very easy to change. We¹ve been running in production for a few months and we use the option to make it easier to disable at build time for testing. Ideally no one will need to enable an option to make something safer or faster if there are limited consequences. Unless there is a request for enable-signals I will plan to update the patch without it before a commit. BTW I'm not sure about committing it into 5.3. It's a question to RM(s). The patch is against 5.3 for convenience and I also have this patched against 5.2.x if anyone would like to try the feature on the latest release. I¹d personally like to see this in 5.3 but I know it¹s little late in the cycle. Whether there is a 5.4 cycle or not might change where this would be best land. Lukas, Johannes? -lucas
Re: [PHP-DEV] [RFC] Zend Signal Handling
Hi! I am proposing the following RFC to improve signal handling in the Zend Engine: http://wiki.php.net/rfc/zendsignals Looks good. If ti works, I don't think we need two signal models - new one would be OK. I'm not sure what happens with win32 though. -- Stanislav Malyshev, Zend Software Architect [EMAIL PROTECTED] http://www.zend.com/ (408)253-8829 MSN: [EMAIL PROTECTED] -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
Stanislav Malyshev wrote: Hi! I am proposing the following RFC to improve signal handling in the Zend Engine: http://wiki.php.net/rfc/zendsignals Looks good. If ti works, I don't think we need two signal models - new one would be OK. I'm not sure what happens with win32 though. Note that this is conceptually based on the Yahoo signal deferring mechanism. The Yahoo signal deferring code is a bit too platform-specific though, but I will walk through and see if any of the many gotchas we deal with need to be fixed in this stuff. Gopal knows this stuff as well. -Rasmus -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Zend Signal Handling
Hi Stas, Looks good. If ti works, I don't think we need two signal models - new one would be OK. I'm not sure what happens with win32 though. This has not been tested on windows but it `should` be unaffacted. If built with ZEND_MAINTAINER_ZTS or if sigaction() is not detected then ZEND_SIGNALS is automatically disabled and the current unsafe blocking functionality will remain. It should work in cygwin environments. Hopefully someone with windows knowledge would be able to determine if there is an alternative way to provide equivalent critical section protection on that platform (will not hold breath though). -lucas -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
[PHP-DEV] [RFC] Zend Signal Handling
Hi Internals, I am proposing the following RFC to improve signal handling in the Zend Engine: http://wiki.php.net/rfc/zendsignals The purpose of zend internal deferred signal handling is to improve the stability of PHP and extensions when running under any forking SAPI. Additionally heavy users of APC or other bytecode caches under Apache 1.x may also see a performance improvement. Please see follow the link above to the complete RFC for details. I look forward to hearing feedback. -lucas -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
