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
