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
