report the real reason when FFI startup fails, and support ffi.enable=preload - #291
Open
wadakatu wants to merge 1 commit into
Open
report the real reason when FFI startup fails, and support ffi.enable=preload#291wadakatu wants to merge 1 commit into
wadakatu wants to merge 1 commit into
Conversation
libraryLoad() caught every FFI\Exception and routed it to Utils::debugLog(), which does nothing unless the user has installed a PSR-3 logger. The caller then threw "Make sure that you've installed libvips", whatever the real cause was. That is what issue libvips#286 hit: FFI had refused to run, but the message pointed at libvips. Keep the last failure message and include it in the exception, so the engine's own explanation reaches the user. That also removes the need to guess at ffi.enable. Under ffi.enable=preload the engine allows an FFI call iff the SAPI is exactly "cli", or the immediate calling function is ZEND_ACC_PRELOADED, or preload compilation is running (php-src ext/ffi/ffi.c:2996-3011), so the same value means usable in one process and dead in another, and ZEND_ACC_PRELOADED is not exposed to PHP. PHP already answers this per call, and now says why when the answer is no. Two consequences: - php-vips works under ffi.enable=preload when php-vips' src is in opcache.preload -- no header file and no FFI::scope needed. FFI::preload() does that from the app's preload script, so the file list is ours rather than something every user has to get right. - "preload" is PHP's compiled-in default (ffi.c:5332) and php.ini-production ships the setting commented out, so php-vips no longer refuses to start on a stock PHP install.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On master, php-vips won't start on a default PHP install:
preloadis PHP's compiled-in default andphp.ini-productionships thesetting commented out, so that's a stock install. FFI works fine there -- the
restriction doesn't apply to the
cliSAPI -- and we throw anyway. 2.6.1started up.
The check can't be right
Under
ffi.enable=preloadPHP allows an FFI call iff the SAPI is exactlycli,or the immediate calling function is
ZEND_ACC_PRELOADED, or preloadcompilation is running (
ext/ffi/ffi.c:2996-3011). So the same value meansusable in one process and dead in another, and
ZEND_ACC_PRELOADEDisn'texposed to PHP.
PHP already answers this per call, and says why when the answer is no. We were
dropping that answer:
libraryLoad()sends every\FFI\ExceptiontoUtils::debugLog(), which does nothing without a PSR-3 logger, and then wethrow "Make sure that you've installed libvips" whatever the cause was. That's
what #286 hit -- the reporter had to attach a debugger to find
FFI API is restricted by "ffi.enable".So this deletes the check and puts PHP's own message in the exception.
FFI::preload()It turns out preloading already works: php-vips'
\FFI::cdef()calls arepermitted as long as its own
srcwas compiled duringopcache.preload. Noheader file, no
FFI::scope, noffi.preload.opcache.preloadtakes onescript and is
INI_SYSTEM, so the app owns that file and we can only becallable from it:
Doing this in the library rather than in the README keeps the file list
complete -- a half-preloaded php-vips starts up and then throws a raw
\FFI\Exception, which extends\Errorand escapescatch (Vips\Exception).It also warms psr/log up for
DebugLogger, and usesscandir(), sinceglob()finds nothing inside a phar.It deliberately doesn't use
FFI::load()/FFI_SCOPE: our declarations arebuilt at runtime from the detected libvips version and the FFI parser has no
preprocessor, so there's no static header to load. If that changes, the header
load belongs in this same method --
FFI::scope()is restricted too, so theclasses need preloading either way.
Verification
php:8.4-cli+libvips428.16.1, aswww-data, overphp -Sso the SAPI iscli-serverand the restriction really applies:ffi.enableopcache.preloadpreload)truepreloadpreloadFFI::preload()0I put a dozen operations through the preloaded path -- thumbnail, file and
buffer load, metadata,
newFromArray,signalConnectprogress,SourceCustom/TargetCustomstreaming -- and the results matchffi.enable=trueexactly.Suite on 8.4.24 and 7.4.33, as root and unprivileged, with
ffi.enable=trueand with no ini at all: 82 tests, 2 skipped, OK.
parallel-lintandphpcsclean. CI green on 7.4 through 8.4.
one-liner to reproduce the preload row
On tests
tests/PreloadTest.phpcoverspreload(). It shells out toPHP_BINARY, sinceopcache.preloadcan only be set at startup.The
ffi.enablehandling has no test. It'sZEND_INI_SYSTEM, and therestriction is disabled outright for the
cliSAPI that PHPUnit runs under(
ffi.c:5462), so it needs a second process on another SAPI. Happy to add aphp -Sharness if you'd like one -- I left it out to keep the diff small.