On Sun, Aug 3, 2008 at 9:34 AM, Lucas Nealan <[EMAIL PROTECTED]> wrote: > Reviewed to my ability and will soon be posting the updated patch for 5_3 > and 6 as well as wiki changes. > > On 8/1/08 6:45 PM, "Arnaud Le Blanc" <[EMAIL PROTECTED]> wrote: >> The ZTS-enabled version of your patch can be found at [1] :) >> Changes: >> Zend Signal Handling is now enabled in ZTS builds. > > Should we be making sure we're not on windows before fully enabling in ZTS > builds? This could also affect platforms that do not have PTHREADS > available. I assume in the latter case we'll compile but standard > sigprocmask will not be be safe enough.
If sigaction is not available Zend Signal Handling will not be enabled, so it will not be enabled on Windows (I assume sigaction is not available on Windows, it is ?). For pthreads and sigprocmask, tsrm_sigmask() can be improved to add support for other APIs. However it seems that only pthread provides that. Apache/APR relies on phtread_sigmask() when available, or sigprocmask() when not. > >> SIGALRM in zend_sigs[] has been replaced by a TIMEOUT_SIG macro which is >> defined to SIGPROF or SIGALRM depending on __CYGWIN__. > > Nice. > >> As sigmasks are thread-specific it is needed to keep sigmask consistent >> across >> all threads. So I just made sigmask a true global (and removed it from the >> module globals). It is initialized at process startup with sigfillset() (and >> not modified after that), so it contains all signals (except SIGILL, SIGSEGV, >> etc). This avoids having to maintain the sigmask each time a signal is >> registered/added and makes the code simpler (and also saves many >> sigprocmask() calls). > > If in the future we have anything in the stack registering a signal handler > via zend_signal this will be problematic because SIGG_*_CRITICAL sections > will not cover the signals registered post-startup. sigaction() is process-wide, so if a thread registers a signal post-startup (this is already the case, zend_sigs[] are registered at request-startup) the signal will be defered in all threads :) > >> However the handling of SIG_DFL is not optimal for now as it needs to reset >> the signal handler to SIG_DFL and call raise(). This is not a problem in >> non-ZTS but in ZTS we must assume that if a signal must be defered, a signal >> handler must be registered in all threads for this signal, or the signal must >> be masked in all threads not having a handler for it. > >> Some changes have been made to use sa_handler or sa_sigaction depending on >> sa_flags as it is unspecified if they points to a union or not. > > It seems pretty safe as I haven't seen mention of a system where this is not > a union. I thought about making these changes anyway but didn't yet. Thanks. > >> The pstorage global have been added to store the queue, declared as an array >> of ZEND_SIGNAL_QUEUE_SIZE. This avoids having to allocate/free it and removes >> the multiple alloc/free for the queue. > > I originally did not use an array as I was hoping to dynamically allocate > more queue at runtime. I hadn't fully thought it out because there are > almost no calls into the code where this would be apropriate. We could maybe > do something in zend_signal_handler_unblock but it might be too late there > and with this it would not be possible. I don't think I have a problem with > this though. > > Not that it won't happen but I can't imagine a scenario where we get > hammered with more signals than we would have slots in > ZEND_SIGNAL_QUEUE_SIZE and it wouldn't be safe to ignore them. At least with > the signals we're handling out of the box. This could change when we get > pcntl working with this. Yes, it may be great to be able to grow the queue when necessary. > >> To allow that to build I had to add some TSRMLS_FETCH() in zend_alloc.c and >> zend_hash.c. This is not optimal but the TSRM argument marcos can probably be >> added to these functions in the future. > > I'll put this in as is and hopefully someone more familiar with those will > be able to review. > >> The tests I made shows that this greatly improves the stability of the >> ZTS-enabled PHP on non-Windows plateforms, however there are some code which >> needs to be protected against interruptions (e.g. zend_llist), or to have a >> wider range protected (zend_hash, zend_alloc). I done that in zend_alloc.c. >> I will send a version for HEAD later. >> [1] http://arnaud.lb.s3.amazonaws.com/zend_signal_1217635857.patch > > I got some build errors when building sapi/cli with and without ZTS. This > seems to come from this change to the original diff in zend_signal.c: > > - zend_closures.c zend_signal.c) > + zend_closures.c) Sorry :) > > I've removed this. I also took out the unused HANDLE_UNBLOCK_INTERRUPTIONS > macro. Feel free to add anything to the wiki you think would be pertinent to > the RFC. > > -lucas > > -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
