Merge "Allow resources to be salvaged across service resets."
authorjenkins-bot <jenkins-bot@gerrit.wikimedia.org>
Thu, 19 May 2016 12:45:16 +0000 (12:45 +0000)
committerGerrit Code Review <gerrit@wikimedia.org>
Thu, 19 May 2016 12:45:16 +0000 (12:45 +0000)
autoload.php
includes/MediaWikiServices.php
includes/Services/SalvageableService.php [new file with mode: 0644]
includes/Services/ServiceContainer.php
includes/Setup.php
includes/config/ConfigFactory.php
tests/phpunit/includes/MediaWikiServicesTest.php
tests/phpunit/includes/Services/ServiceContainerTest.php
tests/phpunit/includes/config/ConfigFactoryTest.php

index aeb69fd..4e8259b 100644 (file)
@@ -852,6 +852,7 @@ $wgAutoloadLocalClasses = [
        'MediaWiki\\Services\\CannotReplaceActiveServiceException' => __DIR__ . '/includes/Services/CannotReplaceActiveServiceException.php',
        'MediaWiki\\Services\\ContainerDisabledException' => __DIR__ . '/includes/Services/ContainerDisabledException.php',
        'MediaWiki\\Services\\DestructibleService' => __DIR__ . '/includes/Services/DestructibleService.php',
+       'MediaWiki\\Services\\SalvageableService' => __DIR__ . '/includes/Services/SalvageableService.php',
        'MediaWiki\\Services\\NoSuchServiceException' => __DIR__ . '/includes/Services/NoSuchServiceException.php',
        'MediaWiki\\Services\\ServiceAlreadyDefinedException' => __DIR__ . '/includes/Services/ServiceAlreadyDefinedException.php',
        'MediaWiki\\Services\\ServiceContainer' => __DIR__ . '/includes/Services/ServiceContainer.php',
index d423212..4028aa2 100644 (file)
@@ -11,6 +11,7 @@ use LBFactory;
 use LinkCache;
 use Liuggio\StatsdClient\Factory\StatsdDataFactory;
 use LoadBalancer;
+use MediaWiki\Services\SalvageableService;
 use MediaWiki\Services\ServiceContainer;
 use MWException;
 use ObjectCache;
@@ -90,7 +91,7 @@ class MediaWikiServices extends ServiceContainer {
                        // even if it's just a file name or database credentials to load
                        // configuration from.
                        $bootstrapConfig = new GlobalVarConfig();
-                       self::$instance = self::newInstance( $bootstrapConfig );
+                       self::$instance = self::newInstance( $bootstrapConfig, 'load' );
                }
 
                return self::$instance;
@@ -123,7 +124,7 @@ class MediaWikiServices extends ServiceContainer {
        /**
         * Creates a new instance of MediaWikiServices and sets it as the global default
         * instance. getInstance() will return a different MediaWikiServices object
-        * after every call to resetGlobalServiceLocator().
+        * after every call to resetGlobalInstance().
         *
         * @since 1.28
         *
@@ -131,7 +132,7 @@ class MediaWikiServices extends ServiceContainer {
         * when the configuration has changed significantly since bootstrap time, e.g.
         * during the installation process or during testing.
         *
-        * @warning Calling resetGlobalServiceLocator() may leave the application in an inconsistent
+        * @warning Calling resetGlobalInstance() may leave the application in an inconsistent
         * state. Calling this is only safe under the ASSUMPTION that NO REFERENCE to
         * any of the services managed by MediaWikiServices exist. If any service objects
         * managed by the old MediaWikiServices instance remain in use, they may INTERFERE
@@ -152,11 +153,14 @@ class MediaWikiServices extends ServiceContainer {
         *        was no previous instance, a new GlobalVarConfig object will be used to
         *        bootstrap the services.
         *
+        * @param string $quick Set this to "quick" to allow expensive resources to be re-used.
+        * See SalvageableService for details.
+        *
         * @throws MWException If called after MW_SERVICE_BOOTSTRAP_COMPLETE has been defined in
         *         Setup.php (unless MW_PHPUNIT_TEST or MEDIAWIKI_INSTALL or RUN_MAINTENANCE_IF_MAIN
         *          is defined).
         */
-       public static function resetGlobalInstance( Config $bootstrapConfig = null ) {
+       public static function resetGlobalInstance( Config $bootstrapConfig = null, $quick = '' ) {
                if ( self::$instance === null ) {
                        // no global instance yet, nothing to reset
                        return;
@@ -168,9 +172,38 @@ class MediaWikiServices extends ServiceContainer {
                        $bootstrapConfig = self::$instance->getBootstrapConfig();
                }
 
-               self::$instance->destroy();
+               $oldInstance = self::$instance;
 
                self::$instance = self::newInstance( $bootstrapConfig );
+               self::$instance->importWiring( $oldInstance, [ 'BootstrapConfig' ] );
+
+               if ( $quick === 'quick' ) {
+                       self::$instance->salvage( $oldInstance );
+               } else {
+                       $oldInstance->destroy();
+               }
+
+       }
+
+       /**
+        * Salvages the state of any salvageable service instances in $other.
+        *
+        * @note $other will have been destroyed when salvage() returns.
+        *
+        * @param MediaWikiServices $other
+        */
+       private function salvage( self $other ) {
+               foreach ( $this->getServiceNames() as $name ) {
+                       $oldService = $other->peekService( $name );
+
+                       if ( $oldService instanceof SalvageableService ) {
+                               /** @var SalvageableService $newService */
+                               $newService = $this->getService( $name );
+                               $newService->salvage( $oldService );
+                       }
+               }
+
+               $other->destroy();
        }
 
        /**
@@ -179,21 +212,23 @@ class MediaWikiServices extends ServiceContainer {
         * ServiceWiringFiles setting are loaded, and the MediaWikiServices hook is called.
         *
         * @param Config|null $bootstrapConfig The Config object to be registered as the
-        *        'BootstrapConfig' service. This has to contain at least the information
-        *        needed to set up the 'ConfigFactory' service. If not provided, any call
-        *        to getBootstrapConfig(), getConfigFactory, or getMainConfig will fail.
-        *        A MediaWikiServices instance without access to configuration is called
-        *        "primordial".
+        *        'BootstrapConfig' service.
+        *
+        * @param string $loadWiring set this to 'load' to load the wiring files specified
+        *        in the 'ServiceWiringFiles' setting in $bootstrapConfig.
         *
         * @return MediaWikiServices
         * @throws MWException
+        * @throws \FatalError
         */
-       private static function newInstance( Config $bootstrapConfig ) {
+       private static function newInstance( Config $bootstrapConfig, $loadWiring = '' ) {
                $instance = new self( $bootstrapConfig );
 
                // Load the default wiring from the specified files.
-               $wiringFiles = $bootstrapConfig->get( 'ServiceWiringFiles' );
-               $instance->loadWiringFiles( $wiringFiles );
+               if ( $loadWiring === 'load' ) {
+                       $wiringFiles = $bootstrapConfig->get( 'ServiceWiringFiles' );
+                       $instance->loadWiringFiles( $wiringFiles );
+               }
 
                // Provide a traditional hook point to allow extensions to configure services.
                Hooks::run( 'MediaWikiServices', [ $instance ] );
diff --git a/includes/Services/SalvageableService.php b/includes/Services/SalvageableService.php
new file mode 100644 (file)
index 0000000..a613050
--- /dev/null
@@ -0,0 +1,58 @@
+<?php
+namespace MediaWiki\Services;
+
+/**
+ * Interface for salvageable services.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ * http://www.gnu.org/copyleft/gpl.html
+ *
+ * @file
+ *
+ * @since 1.28
+ */
+
+/**
+ * SalvageableService defines an interface for services that are able to salvage state from a
+ * previous instance of the same class. The intent is to allow new service instances to re-use
+ * resources that would be expensive to re-create, such as cached data or network connections.
+ *
+ * @note There is no expectation that services will be destroyed when the process (or web request)
+ * terminates.
+ */
+interface SalvageableService {
+
+       /**
+        * Re-uses state from $other. $other must not be used after being passed to salvage(),
+        * and should be considered to be destroyed.
+        *
+        * @note Implementations are responsible for determining what parts of $other can be re-used
+        * safely. In particular, implementations should check that the relevant configuration of
+        * $other is the same as in $this before re-using resources from $other.
+        *
+        * @note Implementations must take care to detach any re-used resources from the original
+        * service instance. If $other is destroyed later, resources that are now used by the
+        * new service instance must not be affected.
+        *
+        * @note If $other is a DestructibleService, implementations should make sure that $other
+        * is in destroyed state after salvage finished. This may be done by calling $other->destroy()
+        * after carefully detaching all relevant resources.
+        *
+        * @param SalvageableService $other The object to salvage state from. $other must have the
+        * exact same type as $this.
+        */
+       public function salvage( SalvageableService $other );
+
+}
index 66ee918..b336795 100644 (file)
@@ -55,6 +55,11 @@ class ServiceContainer implements DestructibleService {
         */
        private $serviceInstantiators = [];
 
+       /**
+        * @var boolean[] disabled status, per service name
+        */
+       private $disabled = [];
+
        /**
         * @var array
         */
@@ -126,6 +131,28 @@ class ServiceContainer implements DestructibleService {
                }
        }
 
+       /**
+        * Imports all wiring defined in $container. Wiring defined in $container
+        * will override any wiring already defined locally. However, already
+        * existing service instances will be preserved.
+        *
+        * @since 1.28
+        *
+        * @param ServiceContainer $container
+        * @param string[] $skip A list of service names to skip during import
+        */
+       public function importWiring( ServiceContainer $container, $skip = [] ) {
+               $newInstantiators = array_diff_key(
+                       $container->serviceInstantiators,
+                       array_flip( $skip )
+               );
+
+               $this->serviceInstantiators = array_merge(
+                       $this->serviceInstantiators,
+                       $newInstantiators
+               );
+       }
+
        /**
         * Returns true if a service is defined for $name, that is, if a call to getService( $name )
         * would return a service instance.
@@ -220,6 +247,7 @@ class ServiceContainer implements DestructibleService {
                }
 
                $this->serviceInstantiators[$name] = $instantiator;
+               unset( $this->disabled[$name] );
        }
 
        /**
@@ -244,9 +272,7 @@ class ServiceContainer implements DestructibleService {
        public function disableService( $name ) {
                $this->resetService( $name );
 
-               $this->redefineService( $name, function() use ( $name ) {
-                       throw new ServiceDisabledException( $name );
-               } );
+               $this->disabled[$name] = true;
        }
 
        /**
@@ -282,6 +308,7 @@ class ServiceContainer implements DestructibleService {
                }
 
                unset( $this->services[$name] );
+               unset( $this->disabled[$name] );
        }
 
        /**
@@ -299,7 +326,8 @@ class ServiceContainer implements DestructibleService {
         * @param string $name The service name
         *
         * @throws NoSuchServiceException if $name is not a known service.
-        * @throws ServiceDisabledException if this container has already been destroyed.
+        * @throws ContainerDisabledException if this container has already been destroyed.
+        * @throws ServiceDisabledException if the requested service has been disabled.
         *
         * @return object The service instance
         */
@@ -308,6 +336,10 @@ class ServiceContainer implements DestructibleService {
                        throw new ContainerDisabledException();
                }
 
+               if ( isset( $this->disabled[$name] ) ) {
+                       throw new ServiceDisabledException( $name );
+               }
+
                if ( !isset( $this->services[$name] ) ) {
                        $this->services[$name] = $this->createService( $name );
                }
@@ -327,6 +359,7 @@ class ServiceContainer implements DestructibleService {
                                $this->serviceInstantiators[$name],
                                array_merge( [ $this ], $this->extraInstantiationParams )
                        );
+                       // NOTE: when adding more wiring logic here, make sure copyWiring() is kept in sync!
                } else {
                        throw new NoSuchServiceException( $name );
                }
index e57b96a..5b19b5f 100644 (file)
@@ -517,7 +517,7 @@ if ( !class_exists( 'AutoLoader' ) ) {
 
 // Reset the global service locator, so any services that have already been created will be
 // re-created while taking into account any custom settings and extensions.
-MediaWikiServices::resetGlobalInstance( new GlobalVarConfig() );
+MediaWikiServices::resetGlobalInstance( new GlobalVarConfig(), 'quick' );
 
 // Define a constant that indicates that the bootstrapping of the service locator
 // is complete.
index 09b0baa..cd25352 100644 (file)
  *
  * @file
  */
+use MediaWiki\Services\SalvageableService;
+use Wikimedia\Assert\Assert;
 
 /**
  * Factory class to create Config objects
  *
  * @since 1.23
  */
-class ConfigFactory {
+class ConfigFactory implements SalvageableService {
 
        /**
         * Map of config name => callback
@@ -50,6 +52,41 @@ class ConfigFactory {
                return \MediaWiki\MediaWikiServices::getInstance()->getConfigFactory();
        }
 
+       /**
+        * Re-uses existing Cache objects from $other. Cache objects are only re-used if the
+        * registered factory function for both is the same. Cache config is not copied,
+        * and only instances of caches defined on this instance with the same config
+        * are copied.
+        *
+        * @see SalvageableService::salvage()
+        *
+        * @param SalvageableService $other The object to salvage state from. $other must have the
+        * exact same type as $this.
+        */
+       public function salvage( SalvageableService $other ) {
+               Assert::parameterType( self::class, $other, '$other' );
+
+               /** @var ConfigFactory $other */
+               foreach ( $other->factoryFunctions as $name => $otherFunc ) {
+                       if ( !isset( $this->factoryFunctions[$name] ) ) {
+                               continue;
+                       }
+
+                       // if the callback function is the same, salvage the Cache object
+                       // XXX: Closures are never equal!
+                       if ( isset( $other->configs[$name] )
+                               && $this->factoryFunctions[$name] == $otherFunc
+                       ) {
+                               $this->configs[$name] = $other->configs[$name];
+                               unset( $other->configs[$name] );
+                       }
+               }
+
+               // disable $other
+               $other->factoryFunctions = [];
+               $other->configs = [];
+       }
+
        /**
         * @return string[]
         */
@@ -67,23 +104,11 @@ class ConfigFactory {
         * @throws InvalidArgumentException If an invalid callback is provided
         */
        public function register( $name, $callback ) {
-               if ( $callback instanceof Config ) {
-                       $instance = $callback;
-
-                       // Register a callback anyway, for consistency. Note that getConfigNames()
-                       // relies on $factoryFunctions to have all config names.
-                       $callback = function() use ( $instance ) {
-                               return $instance;
-                       };
-               } else {
-                       $instance = null;
-               }
-
-               if ( !is_callable( $callback ) ) {
+               if ( !is_callable( $callback ) && !( $callback instanceof Config ) ) {
                        throw new InvalidArgumentException( 'Invalid callback provided' );
                }
 
-               $this->configs[$name] = $instance;
+               unset( $this->configs[$name] );
                $this->factoryFunctions[$name] = $callback;
        }
 
@@ -105,7 +130,13 @@ class ConfigFactory {
                        if ( !isset( $this->factoryFunctions[$key] ) ) {
                                throw new ConfigException( "No registered builder available for $name." );
                        }
-                       $conf = call_user_func( $this->factoryFunctions[$key], $this );
+
+                       if ( $this->factoryFunctions[$key] instanceof Config ) {
+                               $conf = $this->factoryFunctions[$key];
+                       } else {
+                               $conf = call_user_func( $this->factoryFunctions[$key], $this );
+                       }
+
                        if ( $conf instanceof Config ) {
                                $this->configs[$name] = $conf;
                        } else {
index a45c3ae..0e646ea 100644 (file)
@@ -2,6 +2,8 @@
 use Liuggio\StatsdClient\Factory\StatsdDataFactory;
 use MediaWiki\Interwiki\InterwikiLookup;
 use MediaWiki\MediaWikiServices;
+use MediaWiki\Services\DestructibleService;
+use MediaWiki\Services\SalvageableService;
 use MediaWiki\Services\ServiceDisabledException;
 
 /**
@@ -9,7 +11,7 @@ use MediaWiki\Services\ServiceDisabledException;
  *
  * @group MediaWiki
  */
-class MediaWikiServicesTest extends PHPUnit_Framework_TestCase {
+class MediaWikiServicesTest extends MediaWikiTestCase {
 
        /**
         * @return Config
@@ -66,9 +68,69 @@ class MediaWikiServicesTest extends PHPUnit_Framework_TestCase {
                $newServices = $this->newMediaWikiServices();
                $oldServices = MediaWikiServices::forceGlobalInstance( $newServices );
 
+               $service1 = $this->getMock( SalvageableService::class );
+               $service1->expects( $this->never() )
+                       ->method( 'salvage' );
+
+               $newServices->defineService(
+                       'Test',
+                       function() use ( $service1 ) {
+                               return $service1;
+                       }
+               );
+
+               // force instantiation
+               $newServices->getService( 'Test' );
+
                MediaWikiServices::resetGlobalInstance( $this->newTestConfig() );
                $theServices = MediaWikiServices::getInstance();
 
+               $this->assertSame(
+                       $service1,
+                       $theServices->getService( 'Test' ),
+                       'service definition should survive reset'
+               );
+
+               $this->assertNotSame( $theServices, $newServices );
+               $this->assertNotSame( $theServices, $oldServices );
+
+               MediaWikiServices::forceGlobalInstance( $oldServices );
+       }
+
+       public function testResetGlobalInstance_quick() {
+               $newServices = $this->newMediaWikiServices();
+               $oldServices = MediaWikiServices::forceGlobalInstance( $newServices );
+
+               $service1 = $this->getMock( SalvageableService::class );
+               $service1->expects( $this->never() )
+                       ->method( 'salvage' );
+
+               $service2 = $this->getMock( SalvageableService::class );
+               $service2->expects( $this->once() )
+                       ->method( 'salvage' )
+                       ->with( $service1 );
+
+               // sequence of values the instantiator will return
+               $instantiatorReturnValues = [
+                       $service1,
+                       $service2,
+               ];
+
+               $newServices->defineService(
+                       'Test',
+                       function() use ( &$instantiatorReturnValues ) {
+                               return array_shift( $instantiatorReturnValues );
+                       }
+               );
+
+               // force instantiation
+               $newServices->getService( 'Test' );
+
+               MediaWikiServices::resetGlobalInstance( $this->newTestConfig(), 'quick' );
+               $theServices = MediaWikiServices::getInstance();
+
+               $this->assertSame( $service2, $theServices->getService( 'Test' ) );
+
                $this->assertNotSame( $theServices, $newServices );
                $this->assertNotSame( $theServices, $oldServices );
 
@@ -110,35 +172,42 @@ class MediaWikiServicesTest extends PHPUnit_Framework_TestCase {
                }
 
                MediaWikiServices::forceGlobalInstance( $oldServices );
+               $newServices->destroy();
        }
 
        public function testResetChildProcessServices() {
                $newServices = $this->newMediaWikiServices();
                $oldServices = MediaWikiServices::forceGlobalInstance( $newServices );
 
-               $lbFactory = $this->getMockBuilder( 'LBFactorySimple' )
-                       ->disableOriginalConstructor()
-                       ->getMock();
+               $service1 = $this->getMock( DestructibleService::class );
+               $service1->expects( $this->once() )
+                       ->method( 'destroy' );
 
-               $lbFactory->expects( $this->once() )
+               $service2 = $this->getMock( DestructibleService::class );
+               $service2->expects( $this->never() )
                        ->method( 'destroy' );
 
-               $newServices->redefineService(
-                       'DBLoadBalancerFactory',
-                       function() use ( $lbFactory ) {
-                               return $lbFactory;
+               // sequence of values the instantiator will return
+               $instantiatorReturnValues = [
+                       $service1,
+                       $service2,
+               ];
+
+               $newServices->defineService(
+                       'Test',
+                       function() use ( &$instantiatorReturnValues ) {
+                               return array_shift( $instantiatorReturnValues );
                        }
                );
 
                // force the service to become active, so we can check that it does get destroyed
-               $oldLBFactory = $newServices->getService( 'DBLoadBalancerFactory' );
+               $oldTestService = $newServices->getService( 'Test' );
 
                MediaWikiServices::resetChildProcessServices();
                $finalServices = MediaWikiServices::getInstance();
 
-               $newLBFactory = $finalServices->getService( 'DBLoadBalancerFactory' );
-
-               $this->assertNotSame( $oldLBFactory, $newLBFactory );
+               $newTestService = $finalServices->getService( 'Test' );
+               $this->assertNotSame( $oldTestService, $newTestService );
 
                MediaWikiServices::forceGlobalInstance( $oldServices );
        }
index 933777c..f22e123 100644 (file)
@@ -166,6 +166,55 @@ class ServiceContainerTest extends PHPUnit_Framework_TestCase {
                $this->assertSame( 'Bar!', $services->getService( 'Bar' ) );
        }
 
+       public function testImportWiring() {
+               $services = $this->newServiceContainer();
+
+               $wiring = [
+                       'Foo' => function() {
+                               return 'Foo!';
+                       },
+                       'Bar' => function() {
+                               return 'Bar!';
+                       },
+                       'Car' => function() {
+                               return 'FUBAR!';
+                       },
+               ];
+
+               $services->applyWiring( $wiring );
+
+               $newServices = $this->newServiceContainer();
+
+               // define a service before importing, so we can later check that
+               // existing service instances survive importWiring()
+               $newServices->defineService( 'Car', function() {
+                       return 'Car!';
+               } );
+
+               // force instantiation
+               $newServices->getService( 'Car' );
+
+               // Define another service, so we can later check that extra wiring
+               // is not lost.
+               $newServices->defineService( 'Xar', function() {
+                       return 'Xar!';
+               } );
+
+               // import wiring, but skip `Bar`
+               $newServices->importWiring( $services, [ 'Bar' ] );
+
+               $this->assertNotContains( 'Bar', $newServices->getServiceNames(), 'Skip `Bar` service' );
+               $this->assertSame( 'Foo!', $newServices->getService( 'Foo' ) );
+
+               // import all wiring, but preserve existing service instance
+               $newServices->importWiring( $services );
+
+               $this->assertContains( 'Bar', $newServices->getServiceNames(), 'Import all services' );
+               $this->assertSame( 'Bar!', $newServices->getService( 'Bar' ) );
+               $this->assertSame( 'Car!', $newServices->getService( 'Car' ), 'Use existing service instance' );
+               $this->assertSame( 'Xar!', $newServices->getService( 'Xar' ), 'Predefined services are kept' );
+       }
+
        public function testLoadWiringFiles() {
                $services = $this->newServiceContainer();
 
@@ -220,6 +269,27 @@ class ServiceContainerTest extends PHPUnit_Framework_TestCase {
                $this->assertSame( $theService1, $services->getService( $name ) );
        }
 
+       public function testRedefineService_disabled() {
+               $services = $this->newServiceContainer( [ 'Foo' ] );
+
+               $theService1 = new stdClass();
+               $name = 'TestService92834576';
+
+               $services->defineService( $name, function() {
+                       return 'Foo';
+               } );
+
+               // disable the service. we should be able to redefine it anyway.
+               $services->disableService( $name );
+
+               $services->redefineService( $name, function() use ( $theService1 ) {
+                       return $theService1;
+               } );
+
+               // force instantiation, check result
+               $this->assertSame( $theService1, $services->getService( $name ) );
+       }
+
        public function testRedefineService_fail_undefined() {
                $services = $this->newServiceContainer();
 
@@ -294,13 +364,6 @@ class ServiceContainerTest extends PHPUnit_Framework_TestCase {
                $this->assertContains( 'Bar', $services->getServiceNames() );
                $this->assertContains( 'Qux', $services->getServiceNames() );
 
-               // re-enable Bar service
-               $services->redefineService( 'Bar', function() {
-                       return new stdClass();
-               } );
-
-               $services->getService( 'Bar' );
-
                $this->setExpectedException( 'MediaWiki\Services\ServiceDisabledException' );
                $services->getService( 'Qux' );
        }
index 2c1d1e6..f42cb95 100644 (file)
@@ -44,6 +44,42 @@ class ConfigFactoryTest extends MediaWikiTestCase {
                $this->assertNotSame( $config1, $config2 );
        }
 
+       /**
+        * @covers ConfigFactory::register
+        */
+       public function testSalvage() {
+               $oldFactory = new ConfigFactory();
+               $oldFactory->register( 'foo', 'GlobalVarConfig::newInstance' );
+               $oldFactory->register( 'bar', 'GlobalVarConfig::newInstance' );
+               $oldFactory->register( 'quux', 'GlobalVarConfig::newInstance' );
+
+               // instantiate two of the three defined configurations
+               $foo = $oldFactory->makeConfig( 'foo' );
+               $bar = $oldFactory->makeConfig( 'bar' );
+               $quux = $oldFactory->makeConfig( 'quux' );
+
+               // define new config instance
+               $newFactory = new ConfigFactory();
+               $newFactory->register( 'foo', 'GlobalVarConfig::newInstance' );
+               $newFactory->register( 'bar', function() {
+                       return new HashConfig();
+               } );
+
+               // "foo" and "quux" are defined in the old and the new factory.
+               // The old factory has instances for "foo" and "bar", but not "quux".
+               $newFactory->salvage( $oldFactory );
+
+               $newFoo = $newFactory->makeConfig( 'foo' );
+               $this->assertSame( $foo, $newFoo, 'existing instance should be salvaged' );
+
+               $newBar = $newFactory->makeConfig( 'bar' );
+               $this->assertNotSame( $bar, $newBar, 'don\'t salvage if callbacks differ' );
+
+               // the new factory doesn't have quux defined, so the quux instance should not be salvaged
+               $this->setExpectedException( 'ConfigException' );
+               $newFactory->makeConfig( 'quux' );
+       }
+
        /**
         * @covers ConfigFactory::register
         */