dev/core#4055 Remove OPCache risk in the extension class loader

[kainuk]

Overview

It is a code thing. The extension class loader used require to load configuration. When OPCache is enabled, this adds the risk that outdated configuration is used, specially when extensions are installed. A use case can be found at https://lab.civicrm.org/dev/core/-/issues/4055

Before

• Use file_put_contents to write class loader configuration.
• Use require to read it.

After

• Use file_put_contents to write class loader configuration.
• Use file_get_contents to read it.
• Lock the configuration file as an extra safety measure.

Technical Details

• With some luck this prevents a number of bugs that only show themselves in production situations (because on developer laptops the OP cache is often disabled.).

Comments

Updating this code directly causes a nasty error because the configuration format is changed. Using cv flush solves this, because it removes the cache files.

[civibot]

(Standard links)

• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[braders]

Updating this code directly causes a nasty error because the configuration format is changed. Using cv flush solves this, because it removes the cache files.

I wonder if the name of the file generated by getCacheFile() should be changed to handle this. (i.e. so that after upgrade, a new cache file will be created automatically, because a cache file won't exist with the new name). Given that the file contents now contain the serialised data alone (not a valid PHP file), it doesn't feel right that the cache file has the .php filetype anyway.

The filename should probably be CachedExtLoader.{hash}.php.meta, which is consistent with the CachedCiviContainer serialised data files.

[kainuk]

Hi @braders, changing the filename is an excellent idea. I will update the PR.

[eileenmcnaughton]

This sounded a bit like https://lab.civicrm.org/dev/core/-/issues/4054 - but perhaps there is no overlap

[kainuk]

This sounded a bit like https://lab.civicrm.org/dev/core/-/issues/4054 - but perhaps there is no overlap

Looks indeed similar, a cache that retrieves an outdated result.

[civicrm-builder]

Can one of the admins verify this patch?

[eileenmcnaughton]

@totten

[totten]

The funny thing is -- using the op-code cache was kind of the point. You can find various authors who suggest caching semi-permanent data as *.php files because op-code caches tend be more performant. Ex:

• https://medium.com/@dylanwenzlau/500x-faster-caching-than-redis-memcache-apc-in-php-hhvm-dcd26e8447ad
• https://www.percona.com/blog/2006/08/09/cache-performance-comparison/

My default position is to assume that opcode cache is better for regular pageloads, although this not certain.

• The benchmarks in the links aren't recent, and they're not benchmarking the same thing. We don't have benchmarks for this specific code.
• The CachedExtLoader is not huge. We're talking about the class-loading rules specifically for CiviCRM extensions and not all classes. On my local, CachedExtLoader.*.php is 4k -- compared to 88k for civicrm/vendor/composer/autoload*.php and 120k for CachedCiviContainer. Micro-optimization of this data-structure may not be as important.
• The implementation here took a short-cut and used unserialize(). I believe most tools that leverage *.php-based caching actually leverage opcodes more (by writing verbose PHP code). Compare CachedExtLoader.*.php with CachedCiviContainer.*.php and vendor/composer/autoload*.php -- at runtime, reading CachedExtLoader requires an additional pass to unserialize. (Of course, alternatives using memcache/redis/sql would also require decoding -- so it's not a fatal short-cut.)

I suppose the ideal is to benchmark.

But let me throw out one alternative -- there was a vaguely similar issue for CachedCiviContainer (where you would get a stale container after toggling extensions). The resolution there still used *.php-based cache for the typical page-load, but the new configuration went into a new cache file. The key bit:

c76bdfc#diff-4f4d71cf8bcad044570b79ff938e54e9695649ee124b96a6fcfc5e1ef36f555f

-    $envId = \CRM_Core_Config_Runtime::getId();
+    $envId = md5(implode(',', array_merge(
+      [\CRM_Core_Config_Runtime::getId()],
+      array_column(\CRM_Extension_System::singleton()->getMapper()->getActiveModuleFiles(), 'prefix')
+    )));
     $file = \Civi::paths()->getPath("[civicrm.compile]/CachedCiviContainer.{$envId}.php");

We could try an analogous change in CRM_Extension_ClassLoader::getCacheFile()?