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
