resourceloader: Add definition hashing to improve cache invalidation

Currently we invalidate module caches based on timestamps from
various places (including message blob, file mtimes and more).

This meant that when a module changes such that the maximum
detectable timestamp is still the same, the cache would not
invalidate.

Module classes can now implement a method to build a summary.
In most cases this should be (a normalised version of) the
definition array originally given to ResourceLoader::register
(such as $wgResourceModules items).

The most common scenarios this addresses:

* File lists (scripts, styles) being re-ordered.
* Files being removed when more recently changed files remain
  part of the module (e.g. removing an older file).
* Files or messages being added to a module while other more
  recently changed things are already part of the module.
  E.g. you create a message and use it in a js file, but forget
  to add the message to the module. Then later you add the msg
  key. This last update would be ignored, because the mtime of
  the message is no newer than the total which already included
  the (same) mtime of the js file added in the previous update.
* Change the concatenation order of pages in a Gadget definition.

Bug: 37812
Change-Id: I00cf086c981a84235623bf58fb83c9c23aa2d619

diff --git a/RELEASE-NOTES-1.23 b/RELEASE-NOTES-1.23
index 2d89dd2..d4c413e 100644 (file)
--- a/RELEASE-NOTES-1.23
+++ b/RELEASE-NOTES-1.23
@@ -53,6 +53,8 @@ production.
 * Classes TitleListDependency and TitleDependency have been removed, as they
   have been found unused in core and extensions for a long time.
 * (bug 57098) SpecialPasswordReset now obeys returnto parameter
+* (bug 37812) ResourceLoader will notice when a module's definition changes and
+  recompile it accordingly.
 
 === API changes in 1.23 ===
 * (bug 54884) action=parse&prop=categories now indicates hidden and missing

diff --git a/includes/resourceloader/ResourceLoaderFileModule.php b/includes/resourceloader/ResourceLoaderFileModule.php
index 8a48478..43330da 100644 (file)
--- a/includes/resourceloader/ResourceLoaderFileModule.php
+++ b/includes/resourceloader/ResourceLoaderFileModule.php
@@ -241,7 +241,11 @@ class ResourceLoaderFileModule extends ResourceLoaderModule {
                                case 'dependencies':
                                case 'messages':
                                case 'targets':
-                                       $this->{$member} = (array)$option;
+                                       // Normalise
+                                       $option = array_values( array_unique( (array)$option ) );
+                                       sort( $option );
+
+                                       $this->{$member} = $option;
                                        break;
                                // Single strings
                                case 'group':
@@ -457,14 +461,49 @@ class ResourceLoaderFileModule extends ResourceLoaderModule {
                wfProfileIn( __METHOD__ . '-filemtime' );
                $filesMtime = max( array_map( array( __CLASS__, 'safeFilemtime' ), $files ) );
                wfProfileOut( __METHOD__ . '-filemtime' );
+
                $this->modifiedTime[$context->getHash()] = max(
                        $filesMtime,
-                       $this->getMsgBlobMtime( $context->getLanguage() ) );
+                       $this->getMsgBlobMtime( $context->getLanguage() ),
+                       $this->getDefinitionMtime( $context )
+               );
 
                wfProfileOut( __METHOD__ );
                return $this->modifiedTime[$context->getHash()];
        }
 
+       /**
+        * Get the definition summary for this module.
+        *
+        * @return Array
+        */
+       public function getDefinitionSummary( ResourceLoaderContext $context ) {
+               $summary = array(
+                       'class' => get_class( $this ),
+               );
+               foreach ( array(
+                       'scripts',
+                       'debugScripts',
+                       'loaderScripts',
+                       'styles',
+                       'languageScripts',
+                       'skinScripts',
+                       'skinStyles',
+                       'dependencies',
+                       'messages',
+                       'targets',
+                       'group',
+                       'position',
+                       'localBasePath',
+                       'remoteBasePath',
+                       'debugRaw',
+                       'raw',
+               ) as $member ) {
+                       $summary[$member] = $this->{$member};
+               };
+               return $summary;
+       }
+
        /* Protected Methods */
 
        /**

diff --git a/includes/resourceloader/ResourceLoaderModule.php b/includes/resourceloader/ResourceLoaderModule.php
index 11264fc..26092af 100644 (file)
--- a/includes/resourceloader/ResourceLoaderModule.php
+++ b/includes/resourceloader/ResourceLoaderModule.php
@@ -398,7 +398,8 @@ abstract class ResourceLoaderModule {
         * Helper method for calculating when the module's hash (if it has one) changed.
         *
         * @param ResourceLoaderContext $context
-        * @return integer: UNIX timestamp or 0 if there is no hash provided
+        * @return integer: UNIX timestamp or 0 if no hash was provided
+        *  by getModifiedHash()
         */
        public function getHashMtime( ResourceLoaderContext $context ) {
                $hash = $this->getModifiedHash( $context );
@@ -425,8 +426,10 @@ abstract class ResourceLoaderModule {
        }
 
        /**
-        * Get the last modification timestamp of the message blob for this
-        * module in a given language.
+        * Get the hash for whatever this module may contain.
+        *
+        * This is the method subclasses should implement if they want to make
+        * use of getHashMTime() inside getModifiedTime().
         *
         * @param ResourceLoaderContext $context
         * @return string|null: Hash
@@ -435,6 +438,79 @@ abstract class ResourceLoaderModule {
                return null;
        }
 
+       /**
+        * Helper method for calculating when this module's definition summary was last changed.
+        *
+        * @return integer: UNIX timestamp or 0 if no definition summary was provided
+        *  by getDefinitionSummary()
+        */
+       public function getDefinitionMtime( ResourceLoaderContext $context ) {
+               wfProfileIn( __METHOD__ );
+               $summary = $this->getDefinitionSummary( $context );
+               if ( $summary === null ) {
+                       return 0;
+               }
+
+               $hash = md5( json_encode( $summary ) );
+
+               $cache = wfGetCache( CACHE_ANYTHING );
+
+               // Embed the hash itself in the cache key. This allows for a few nifty things:
+               // - During deployment, servers with old and new versions of the code communicating
+               //   with the same memcached will not override the same key repeatedly increasing
+               //   the timestamp.
+               // - In case of the definition changing and then changing back in a short period of time
+               //   (e.g. in case of a revert or a corrupt server) the old timestamp and client-side cache
+               //   url will be re-used.
+               // - If different context-combinations (e.g. same skin, same language or some combination
+               //   thereof) result in the same definition, they will use the same hash and timestamp.
+               $key = wfMemcKey( 'resourceloader', 'moduledefinition', $this->getName(), $hash );
+
+               $data = $cache->get( $key );
+               if ( is_int( $data ) && $data > 0 ) {
+                       // We've seen this hash before, re-use the timestamp of when we first saw it.
+                       return $data;
+               }
+
+               wfDebugLog( 'resourceloader', __METHOD__ . ": New definition hash for module {$this->getName()} in context {$context->getHash()}: $hash." );
+
+               $timestamp = time();
+               $cache->set( $key, $timestamp );
+
+               wfProfileOut( __METHOD__ );
+               return $timestamp;
+       }
+
+       /**
+        * Get the definition summary for this module.
+        *
+        * This is the method subclasses should implement if they want to make
+        * use of getDefinitionMTime() inside getModifiedTime().
+        *
+        * Return an array containing values from all significant properties of this
+        * module's definition. Be sure to include things that are explicitly ordered,
+        * in their actaul order (bug 37812).
+        *
+        * Avoid including things that are insiginificant (e.g. order of message
+        * keys is insignificant and should be sorted to avoid unnecessary cache
+        * invalidation).
+        *
+        * Avoid including things already considered by other methods inside your
+        * getModifiedTime(), such as file mtime timestamps.
+        *
+        * Serialisation is done using json_encode, which means object state is not
+        * taken into account when building the hash. This data structure must only
+        * contain arrays and scalars as values (avoid object instances) which means
+        * it requires abstraction.
+        *
+        * @return Array|null
+        */
+       public function getDefinitionSummary( ResourceLoaderContext $context ) {
+               return array(
+                       'class' => get_class( $this ),
+               );
+       }
+
        /**
         * Check whether this module is known to be empty. If a child class
         * has an easy and cheap way to determine that this module is

diff --git a/includes/resourceloader/ResourceLoaderWikiModule.php b/includes/resourceloader/ResourceLoaderWikiModule.php
index 3f10ae5..2653f76 100644 (file)
--- a/includes/resourceloader/ResourceLoaderWikiModule.php
+++ b/includes/resourceloader/ResourceLoaderWikiModule.php
@@ -180,10 +180,26 @@ abstract class ResourceLoaderWikiModule extends ResourceLoaderModule {
                if ( count( $mtimes ) ) {
                        $modifiedTime = max( $modifiedTime, max( $mtimes ) );
                }
-               $modifiedTime = max( $modifiedTime, $this->getMsgBlobMtime( $context->getLanguage() ) );
+               $modifiedTime = max(
+                       $modifiedTime,
+                       $this->getMsgBlobMtime( $context->getLanguage() ),
+                       $this->getDefinitionMtime( $context )
+               );
                return $modifiedTime;
        }
 
+       /**
+        * Get the definition summary for this module.
+        *
+        * @return Array
+        */
+       public function getDefinitionSummary( ResourceLoaderContext $context ) {
+               return array(
+                       'class' => get_class( $this ),
+                       'pages' => $this->getPages( $context ),
+               );
+       }
+
        /**
         * @param $context ResourceLoaderContext
         * @return bool

diff --git a/tests/phpunit/includes/ResourceLoaderModuleTest.php b/tests/phpunit/includes/ResourceLoaderModuleTest.php
new file mode 100644 (file)
index 0000000..4643319
--- /dev/null
+++ b/tests/phpunit/includes/ResourceLoaderModuleTest.php
@@ -0,0 +1,87 @@
+<?php
+
+class ResourceLoaderModuleTest extends MediaWikiTestCase {
+
+       protected static function getResourceLoaderContext() {
+               $resourceLoader = new ResourceLoader();
+               $request = new FauxRequest( array(
+                               'debug' => 'false',
+                               'lang' => 'en',
+                               'modules' => 'startup',
+                               'only' => 'scripts',
+                               'skin' => 'vector',
+               ) );
+               return new ResourceLoaderContext( $resourceLoader, $request );
+       }
+
+       /**
+        * @covers ResourceLoaderModule::getDefinitionSummary
+        * @covers ResourceLoaderFileModule::getDefinitionSummary
+        */
+       public function testDefinitionSummary() {
+               $context = self::getResourceLoaderContext();
+
+               $baseParams = array(
+                       'scripts' => array( 'foo.js', 'bar.js' ),
+                       'dependencies' => array( 'jquery', 'mediawiki' ),
+                       'messages' => array( 'hello', 'world' ),
+               );
+
+               $module = new ResourceLoaderFileModule( $baseParams );
+
+               $jsonSummary = json_encode( $module->getDefinitionSummary( $context ) );
+
+               // Exactly the same
+               $module = new ResourceLoaderFileModule( $baseParams );
+
+               $this->assertEquals(
+                       $jsonSummary,
+                       json_encode( $module->getDefinitionSummary( $context ) ),
+                       'Instance is insignificant'
+               );
+
+               // Re-order dependencies
+               $module = new ResourceLoaderFileModule( array(
+                       'dependencies' => array( 'mediawiki', 'jquery' ),
+               ) + $baseParams );
+
+               $this->assertEquals(
+                       $jsonSummary,
+                       json_encode( $module->getDefinitionSummary( $context ) ),
+                       'Order of dependencies is insignificant'
+               );
+
+               // Re-order messages
+               $module = new ResourceLoaderFileModule( array(
+                       'messages' => array( 'world', 'hello' ),
+               ) + $baseParams );
+
+               $this->assertEquals(
+                       $jsonSummary,
+                       json_encode( $module->getDefinitionSummary( $context ) ),
+                       'Order of messages is insignificant'
+               );
+
+               // Re-order scripts
+               $module = new ResourceLoaderFileModule( array(
+                       'scripts' => array( 'bar.js', 'foo.js' ),
+               ) + $baseParams );
+
+               $this->assertNotEquals(
+                       $jsonSummary,
+                       json_encode( $module->getDefinitionSummary( $context ) ),
+                       'Order of scripts is significant'
+               );
+
+               // Subclass
+               $module = new ResourceLoaderFileModuleTestModule( $baseParams );
+
+               $this->assertNotEquals(
+                       $jsonSummary,
+                       json_encode( $module->getDefinitionSummary( $context ) ),
+                       'Class is significant'
+               );
+       }
+}
+
+class ResourceLoaderFileModuleTestModule extends ResourceLoaderFileModule {}