On 21/10/22 01:11, Kévin Dunglas wrote:
Here is the plan:
1. Switch to timer_create() for timeouts, but only on Linux (it's not
supported on other platforms yet), when ZTS is enabled and Zend Signals is
disabled. As the feature is currently entirely broken when ZTS is on, this
can be considered a bug fix and cannot be worse than it currently is.
1bis. Can be done independently of 1., and even in parallel, optional as
long as we keep the --disable-zend-signals flag: Remove Zend Signals
entirely, because even if it can be partially fixed (I proposed a patch
fixing segfaults: https://github.com/php/php-src/pull/9766), it seems now
useless and causes unfixable issues with some signals such as SIGINT (
https://github.com/php/php-src/issues/9649#issuecomment-1265811930)
2. Switch to Grand Central Dispatch on macOS and FreeBSD when ZTS is
enabled and Zend Signals is disabled (if not removed at this point), which
provides a feature similar to timer_create() for these platforms.
3. Probably in a future major version, optional: switch to
timer_create()/GCD even for non-ZTS builds to uniformize and simplify the
code.
Sounds good to me, for what it's worth.
I spent some time studying PR #5591 since it feels like I should have
some expertise in this area by now. Timeout handling in PHP 5 was a
joke. The list of library functions which can be safely re-entered
from a signal handler is listed on man signal-safety(7). Most notably
it does not include malloc(). You cannot just longjmp() out of a
signal handler and re-enter userspace code, as PHP 5 was doing.
I partly solved this problem just for Wikimedia in 2011 in a
hilariously hacky custom extension (wmerrors). It defensively produced
an error page, guarded by a second timer, then killed the process with
abort(). This helped to reduce the rate of deadlocks and segfaults in
Wikimedia production.
So I was pretty happy when the new system for signal and timeout
handling was introduced in PHP 7.1. I extended that system by writing
the Excimer extension, which provides userspace timer facilities with
timer_create() and vm_interrupt. We're using Excimer in production to
implement timeouts.
My understanding of PR #5591 is that it is removing the remnants of
the old signal handling system. There is a lot of discussion of the
correct implementation of HANDLE_BLOCK_INTERRUPTIONS(), but that macro
was from the outset an inadequate approach to signal safety. Maybe it
helped to reduce the rate of segfaults, but at Wikimedia's scale it
was still a segfault firehose. If your signal handler calls malloc(),
then obviously every malloc() call and every call of a library
function that may call malloc() becomes a critical section.
So in my view, the critical sections should all be removed and all
signal handlers should be implemented safely, by setting a flag that
is checked from a safe context. The same recommendation applies
whether you use timer_create() or setitimer().
In Excimer we are using timer_create() with SIGEV_THREAD. PHP request
threads are registered in a global hashtable with long-lived integer
handles, because there is no way to prevent events from being
delivered after the relevant request has terminated. You can't store a
pointer in sival_ptr if it will be freed at request shutdown.
SIGEV_THREAD is less intrusive than SIGEV_SIGNAL or SIGEV_THREAD_ID
since it doesn't interrupt syscalls.
What do you think about this plan? Apart from the technical aspects, what's
the best way forward? Submit patches? Propose an RFC? Do both? (pardon my
ignorance of internals processes).
Submit PRs I guess. I've only submitted a handful of PRs to PHP, and I
haven't got push access, so I'm not an expert. But I think the usual
strategies apply. Answer the nitpickers, get to know people, nag,
escalate, etc.
(I did have Subversion write access, which I got by taking over
maintainership of a PEAR package, but I was never brave enough to push
things to the PHP core.)
-- Tim Starling
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php
