> On 7/30/08 6:11 AM, "Lukas Kahwe Smith" <[EMAIL PROTECTED]> wrote: > 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? >> On 7/30/08 9:37 AM, "Dmitry Stogov" <[EMAIL PROTECTED]> wrote: >> 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.
Sorry about the delay getting some of these things worked on over the weekend. I had to recover from OSCON and didn't get a chance to look at things until back in the office. Seems as though I've been given a stay and can land in Alpha 2 once some more work is done. Thanks for this. In response to Dmitry's feedback and some others: > On 7/30/08 9:37 AM, "Dmitry Stogov" <[EMAIL PROTECTED]> 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. >> On 7/30/08 9:50 AM, "Rasmus Lerdorf" <[EMAIL PROTECTED]> wrote: >> It may be. But there is really no way around that. That's why we >> talked about having an optional request shutdown check that would tell >> you if something hi-jacked one of the signals so at least you know that >> this is happening. I have to go with what Rasmus said here, theres not much we can do. It seems pretty unlikely since the new signal handler would have to be installed via a SAPI callback. I'm going to go ahead and modify the check in zend_signal_deactivate to be ini configurable and throw a E_CORE_WARNING instead of being enabled via ZEND_DEBUG. This should make it easier to troubleshoot. > On 7/30/08 9:37 AM, "Dmitry Stogov" <[EMAIL PROTECTED]> wrote: > 2) It is incompatible with ext/pcntl >> On 7/30/08 11:15 AM, "Arnaud Le Blanc" <[EMAIL PROTECTED]> wrote: >> 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. I made some notes about this compatibility in the RFC as using pcntl with this won't break anything, we'll just potentially lose some protections. I would like to see both working together but for the initial version I'm not convinced this should be a requirement. > 3) It breaks 3 tests (in debug mode) > tests/classes/destructor_visibility_001.phpt > tests/func/005a.phpt >> 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 ;) I was pretty sure I had run the test but apparently not. I'll take a look at these and fix right away. Thanks for the pointers Arnaud. We may also want develop some basic tests that we can run against the new functionality. Seems tricky though so I haven't really thought about this much yet. > 4) I would also see the patch for HEAD (to commit them together) This does need to happen but I'm unsure whether committing together is benificial. PHP6 isn't in my future yet so I haven't had an opportunity to work with it. I'll happily port this to 6 but it will take some time before I can do that. If it's generally believed to be a condition for inclusion into 5.3, and unless someone steps in to do the port right away, the whole shebang will need to be scheduled into the next 5.x release. What is the general opinion about this? Thanks for the feedback! -lucas -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
