Page MenuHomePhabricator

LocalisationCache::readPHPFile may read outdated files on PHP 7 in stock MediaWiki (apc.cache_by_default no longer supported)
Closed, ResolvedPublic

Description

In LocalisationCache.php,

		// Disable APC caching
		Wikimedia\suppressWarnings();
		$_apcEnabled = ini_set( 'apc.cache_by_default', '0' );
		Wikimedia\restoreWarnings();

		include $_fileName;

		Wikimedia\suppressWarnings();
		ini_set( 'apc.cache_by_default', $_apcEnabled );
		Wikimedia\restoreWarnings();

Except I can't find any place in php-apcu where that option is read from (did a quick grep), nor does it show up on phpinfo(), so I suspect it may have never been ported over in the apc->apcu migration?

Event Timeline

Legoktm created this task.Oct 15 2018, 7:22 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 15 2018, 7:22 AM
Reedy added a subscriber: Reedy.Oct 15 2018, 8:40 AM

http://php.net/manual/en/apc.configuration.php list cache_by_default, but http://php.net/manual/en/apcu.configuration.php doesn't. So you'd look to be right

Can't obviously see HHVM uses it either

Krinkle triaged this task as High priority.EditedMay 13 2019, 10:41 PM
Krinkle moved this task from Untriaged to LocalisationCache on the MediaWiki-Cache board.
Krinkle added a subscriber: Krinkle.

This option relates to part of apc that related to caching of PHP compilation (previously known as "apc", now known as "opcache"). It was not related to the part of apc we now know as apcu.

On by default, but can be set to off and used in conjunction with positive apc.filters so that files are only cached if matched by a positive filter.

I do not think opcache has a way to disable itself. However, if LocalisationCache depends on that, this part of it will need to refactored to work in a different way so that it doesn't rely on being able to disable opcache at run-time.

As of writing, this aspect of LocalisationCache is presumed broken on all versions of PHP 7. Proposing to require a fix before MW 1.34.

Does not block WMF migration to PHP 7, because the method in question (LocalisationCache::readPHPFile) is, as far as I know, only used when populating the localisation cache. On MediaWiki core by default, and for third-parties, this is something that can happen during a web request.

For WMF, we only execute this part of the code from the generateLocalisationCache maintenance script. And in PHP 7 by default, the opcache.enable_cli option is Off. And is also off in WMF production as far as I can see. This means, the opcache is disabled when we generate localisation and thus this code will accidentally work as intended for WMF.

Krinkle renamed this task from apc.cache_by_default doesn't look used anymore to LocalisationCache::readPHPFile may read outdated files on PHP 7 in stock MediaWiki (apc.cache_by_default no longer supported).May 13 2019, 10:42 PM
Reedy moved this task from Backlog to MediaWiki core on the PHP 7.3 support board.Jun 8 2019, 11:45 PM
Reedy moved this task from Backlog to MediaWiki core on the PHP 7.0 support board.
Reedy moved this task from Backlog to MediaWiki core on the PHP 7.1 support board.

Hey there, should this be moved to 1.35? The cut is a couple of weeks away. If it needs to go out in 1.34, is there anything I can do to help get it out of the door?

fwiw Language-Team says that MediaWiki-Internationalization is "Passive maintenance or unsupported". That means only UBN! tickets will be looked at if even noticed.

Krinkle added a comment.EditedSep 16 2019, 11:23 PM

@Nikerabbit The code in question is broken under PHP 7.2 ("apc" no longer exists, there is "opcache" and "apcu" now).

If this code is no longer needed, please indicate approval toward its removal (I can do it for you), or turn into a UBN because this would soon be broken in prod, possibly already in some cases. And also because it means we can't sign off on PHP 7.2 support for the 1.34 release.

I don't know really. It was changed by @tstarling in rMWd26d659ef1b1: Fix disabling of APC cache when loading message files: apc.enabled has been… and originally added in rMW23cfebd3d25f: * Introduced a new system for localisation caching. The system is based around….

As far as I can see, its purpose was to reduce memory usage by not caching the files (I guess because of our own cache they would not be read again/often), not to avoid stale reads.

Krinkle claimed this task.Sep 21 2019, 12:48 AM

OK. I'll just remove it then. It hasn't worked for a while, and we're not aware of a regression in this area.

Change 538357 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] localisation: Remove PHP5-specific perf optimisation

https://gerrit.wikimedia.org/r/538357

Change 538357 merged by jenkins-bot:
[mediawiki/core@master] localisation: Remove PHP5-specific perf optimisation

https://gerrit.wikimedia.org/r/538357

I guess this is now resolved?

FTR, opcache can be disabled via the INI setting opcache.enable. AFAICS, that will only work for the current request (which is good), and for scripts not yet cached. But there's also opcache_invalidate for the latter.

Krinkle closed this task as Resolved.Sep 23 2019, 4:25 PM

Yep, all good now. Thanks.