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
