Fix CI

[kleisauke]

Temporary workaround, feel free to debug further.

[jcupitt]

Ooop! We crossed over. I'll reenable 8.3.

[jcupitt]

I made a dockerfile for local testing:

https://github.com/jcupitt/docker-builds/tree/master/php-vips-php8.3

I tried some different values for the php max stacksize, but it fails with even 100gb, hilariously:

PHP Warning:  Uncaught Error: Maximum call stack size of 99999950848 bytes (zend.max_allowed_stack_size - zend.reserved_stack_size) reached. Infinite recursion? in /data/php-vips/src/GObject.php:202

But using -1 works fine and (obviously) does not use that much stack.

Perhaps php 8.3.0 is not detecting stack overflow correctly for closures?

[jcupitt]

@uuf6429 can I ping you on this? Any ideas?

[jcupitt]

@dmitryuk I think this mystery is the only thing blocking a new release. If you have any ideas ...

[jcupitt]

Quick summary for anyone who hasn't been following this:

composer test fails for php 8.3 with this stack trace:

> phpunit
PHPUnit 9.6.16 by Sebastian Bergmann and contributors.

...........................................................PHP Warning:  Uncaught Error: Maximum call stack size of 99999950848 bytes (zend.max_allowed_stack_size - zend.reserved_stack_size) reached. Infinite recursion? in /data/php-vips/src/GObject.php:202
Stack trace:
#0 /data/php-vips/src/VipsOperation.php(308): Jcupitt\Vips\GObject::Jcupitt\Vips\{closure}()
#1 /data/php-vips/src/VipsOperation.php(308): FFI->vips_cache_operation_build()
#2 /data/php-vips/src/VipsOperation.php(371): Jcupitt\Vips\VipsOperation::callBase()
#3 /data/php-vips/src/Image.php(1133): Jcupitt\Vips\VipsOperation::call()
#4 /data/php-vips/tests/StreamingTest.php(46): Jcupitt\Vips\Image->writeToTarget()
#5 /data/php-vips/vendor/phpunit/phpunit/src/Framework/TestCase.php(1612): Jcupitt\Vips\Test\StreamingTest->testFromSourceToTarget()
#6 /data/php-vips/vendor/phpunit/phpunit/src/Framework/TestCase.php(1218): PHPUnit\Framework\TestCase->runTest()
#7 /data/php-vips/vendor/phpunit/phpunit/src/Framework/TestResult.php(728): PHPUnit\Framework\TestCase->runBare()
#8 /data/php-vips/vendor/phpunit/phpunit/src/Framework/TestCase.php(968): PHPUnit\Framework\TestResult->run()
#9 /data/php-vips/vendor/phpunit/phpunit/src/Framework/TestSuite.php(684): PHPUnit\Framework\TestCase->run()
#10 /data/php-vips/vendor/phpunit/phpunit/src/Framework/TestSuite.php(684): PHPUnit\Framework\TestSuite->run()
#11 /data/php-vips/vendor/phpunit/phpunit/src/Framework/TestSuite.php(684): PHPUnit\Framework\TestSuite->run()
#12 /data/php-vips/vendor/phpunit/phpunit/src/Framework/TestSuite.php(684): PHPUnit\Framework\TestSuite->run()
#13 /data/php-vips/vendor/phpunit/phpunit/src/TextUI/TestRunner.php(651): PHPUnit\Framework\TestSuite->run()
#14 /data/php-vips/vendor/phpunit/phpunit/src/TextUI/Command.php(144): PHPUnit\TextUI\TestRunner->run()
#15 /data/php-vips/vendor/phpunit/phpunit/src/TextUI/Command.php(97): PHPUnit\TextUI\Command->run()
#16 /data/php-vips/vendor/phpunit/phpunit/phpunit(107): PHPUnit\TextUI\Command::main()
#17 /data/php-vips/vendor/bin/phpunit(120): include('...')
#18 {main}
  thrown in /data/php-vips/src/GObject.php on line 202
PHP Fatal error:  Throwing from FFI callbacks is not allowed in /data/php-vips/src/VipsOperation.php on line 308
Script phpunit handling the test event returned with error code 255

It looks like a stack overflow is being detected in the write marshaller:

https://github.com/libvips/php-vips/blob/master/src/GObject.php#L203

It's when that closure is invoked (before any code runs). Kleis discovered that adding:

zend.max_allowed_stack_size=-1

(ie. no max stack size) to php.ini fixes it. There doesn't seem to be a positive value for max_allowed_stack_size that works.

[kleisauke]

PR php/php-src#12823 might be relevant here. Specifically comment php/php-src#12823 (comment) which mentions that the stack tracking code won't work for FFI callbacks that are called from non-main threads.

[jcupitt]

I agree, that sounds like the issue --- callbacks happening off the main thread. Good find!

I don't think we can work around this one easily.

I suppose we'll need to add zend.max_allowed_stack_size=-1 to our list of required php.ini settings. Maybe something like:

        if (PHP_MAJOR_VERSION >= 8 && PHP_MINOR_VERSION >= 3) {
            $stacksize = ini_get('zend.max_allowed_stack_size');
            if ($stacksize != '-1') {
                throw new Exception("zend.max_allowed_stack_size not set to '-1'");
            }
        }

in startup?

[jcupitt]

.... I added a version of that check, and updated the README.

[uuf6429]

@uuf6429 can I ping you on this? Any ideas?

I'm sorry, I was also clueless at that point.

The problem seemed to happen only for PHP resources being tested.

I started looking at PHP's FFI implementation and then kinda gave up after finding and fixing the (easy) assertion stuff. 😅

It might help to get someone from room 11 to look into it.

I don't think we can work around this one easily.

I think the check you propose is a reasonable one.
My only concern is that releasing it in composer means these packages will not work with future PHP 8.3+ versions (that might have been fixed). On the other hand, using a fixed PHP version or range (e.g. 8.3.0...8.3.x) will cause the opposite problem; packages will not work with future PHP releases without the fix.

The only sane solution is that you keep the check as is, but support disabling it with a flag, e.g.:

class FFI {
    /**
     * <some useful description>
     */
    public static $checkMaxStackSize = true;

    ...

        if (self::$checkMaxStackSize && version_compare(PHP_VERSION, '8.3', '>=') &&
            ini_get('zend.max_allowed_stack_size') != '-1') {
            throw new Exception("zend.max_allowed_stack_size not set to '-1'");
        }

Alternatively, but it might be too much, you could postpone that check to where it is really needed - so it doesn't fully block everyone.

PS: It might be useful to mention this GH issue number in the exception message.

[jcupitt]

My only concern is that releasing it in composer means these packages will not work with future PHP 8.3+ versions (that might have been fixed). On the other hand, using a fixed PHP version or range (e.g. 8.3.0...8.3.x) will cause the opposite problem; packages will not work with future PHP releases without the fix.

If PHP fix this in 8.4 or 8.5, we can do a new php-vips with a range check, so I think it's OK.

This code will still work even if php fix the bug, it'll just mean people have to add a line to their php.ini.

[jcupitt]

I asked for second opinions in room11 btw

[ckilb]

@jcupitt I like to ask if we could add the check. I'm happily using your library in one of my projects with PHP 8.3. Unfortunately I can't change the ini value and it's not possible to temporary change it on runtime.
When I remove the condition for zend.max_allowed_stack_size in your source code, your library is still working for me. So I assume the PHP FFI bug you mentioned is either not occurring on every platform and environment - or it was already fixed by PHP in a minor version.

[kleisauke]

@ckilb AFAIK, this issue occurs only in GObject::signalConnect(), which is required by SourceCustom and/or TargetCustom. If you're not using signalConnect or SourceCustom/TargetCustom it's probably safe to remove this check.

That said, I can confirm that this issue persists in PHP 8.3 and the upcoming 8.4 release, see:
https://github.com/kleisauke/php-vips/actions/runs/11121526904

[ckilb]

@kleisauke You're right, I neither use signalConnect, SourceCustom nor TargetCustom.

[kleisauke]

PR #261 moves this zend.max_allowed_stack_size=-1 check to GObject::signalConnect() instead.