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
