Re: [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 mls@pooteeweet.org

-- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php