SessionManager: Don't save non-persisted sessions to backend storage

This introduces an in-process cache (using a HashBagOStuff) for session
data, and only saves to the external cache when the session is
persisted.

Bug: T125267
Change-Id: Ie161e0f7522cd68515b060ad8cf8c151b7198b0b

diff --git a/includes/session/SessionBackend.php b/includes/session/SessionBackend.php
index f86daaa..2a13ed2 100644 (file)
--- a/includes/session/SessionBackend.php
+++ b/includes/session/SessionBackend.php
@@ -65,7 +65,9 @@ final class SessionBackend {
        private $dataHash = null;
 
        /** @var BagOStuff */
-       private $store;
+       private $tempStore;
+       /** @var BagOStuff */
+       private $permStore;
 
        /** @var LoggerInterface */
        private $logger;
@@ -97,12 +99,14 @@ final class SessionBackend {
        /**
         * @param SessionId $id Session ID object
         * @param SessionInfo $info Session info to populate from
-        * @param BagOStuff $store Backend data store
+        * @param BagOStuff $tempStore In-process data store
+        * @param BagOStuff $permstore Backend data store for persisted sessions
         * @param LoggerInterface $logger
         * @param int $lifetime Session data lifetime in seconds
         */
        public function __construct(
-               SessionId $id, SessionInfo $info, BagOStuff $store, LoggerInterface $logger, $lifetime
+               SessionId $id, SessionInfo $info, BagOStuff $tempStore, BagOStuff $permStore,
+               LoggerInterface $logger, $lifetime
        ) {
                $phpSessionHandling = \RequestContext::getMain()->getConfig()->get( 'PHPSessionHandling' );
                $this->usePhpSessionHandling = $phpSessionHandling !== 'disable';
@@ -121,7 +125,8 @@ final class SessionBackend {
 
                $this->id = $id;
                $this->user = $info->getUserInfo() ? $info->getUserInfo()->getUser() : new User;
-               $this->store = $store;
+               $this->tempStore = $tempStore;
+               $this->permStore = $permStore;
                $this->logger = $logger;
                $this->lifetime = $lifetime;
                $this->provider = $info->getProvider();
@@ -130,7 +135,14 @@ final class SessionBackend {
                $this->forceHTTPS = $info->forceHTTPS();
                $this->providerMetadata = $info->getProviderMetadata();
 
-               $blob = $store->get( wfMemcKey( 'MWSession', (string)$this->id ) );
+               $key = wfMemcKey( 'MWSession', (string)$this->id );
+               $blob = $tempStore->get( $key );
+               if ( $blob === false ) {
+                       $blob = $permStore->get( $key );
+                       if ( $blob !== false ) {
+                               $tempStore->set( $key, $blob );
+                       }
+               }
                if ( !is_array( $blob ) ||
                        !isset( $blob['metadata'] ) || !is_array( $blob['metadata'] ) ||
                        !isset( $blob['data'] ) || !is_array( $blob['data'] )
@@ -229,7 +241,8 @@ final class SessionBackend {
                        $this->autosave();
 
                        // Delete the data for the old session ID now
-                       $this->store->delete( wfMemcKey( 'MWSession', $oldId ) );
+                       $this->tempStore->delete( wfMemcKey( 'MWSession', $oldId ) );
+                       $this->permStore->delete( wfMemcKey( 'MWSession', $oldId ) );
                }
        }
 
@@ -613,7 +626,7 @@ final class SessionBackend {
                        }
                }
 
-               $this->store->set(
+               $this->tempStore->set(
                        wfMemcKey( 'MWSession', (string)$this->id ),
                        array(
                                'data' => $this->data,
@@ -621,6 +634,16 @@ final class SessionBackend {
                        ),
                        $metadata['expires']
                );
+               if ( $this->persist ) {
+                       $this->permStore->set(
+                               wfMemcKey( 'MWSession', (string)$this->id ),
+                               array(
+                                       'data' => $this->data,
+                                       'metadata' => $metadata,
+                               ),
+                               $metadata['expires']
+                       );
+               }
 
                $this->metaDirty = false;
                $this->dataDirty = false;

diff --git a/includes/session/SessionManager.php b/includes/session/SessionManager.php
index 06a765c..0441137 100644 (file)
--- a/includes/session/SessionManager.php
+++ b/includes/session/SessionManager.php
@@ -55,7 +55,10 @@ final class SessionManager implements SessionManagerInterface {
        private $config;
 
        /** @var BagOStuff|null */
-       private $store;
+       private $tempStore;
+
+       /** @var BagOStuff|null */
+       private $permStore;
 
        /** @var SessionProvider[] */
        private $sessionProviders = null;
@@ -159,16 +162,17 @@ final class SessionManager implements SessionManagerInterface {
                        $this->setLogger( \MediaWiki\Logger\LoggerFactory::getInstance( 'session' ) );
                }
 
+               $this->tempStore = new \HashBagOStuff;
                if ( isset( $options['store'] ) ) {
                        if ( !$options['store'] instanceof BagOStuff ) {
                                throw new \InvalidArgumentException(
                                        '$options[\'store\'] must be an instance of BagOStuff'
                                );
                        }
-                       $this->store = $options['store'];
+                       $this->permStore = $options['store'];
                } else {
-                       $this->store = \ObjectCache::getInstance( $this->config->get( 'SessionCacheType' ) );
-                       $this->store->setLogger( $this->logger );
+                       $this->permStore = \ObjectCache::getInstance( $this->config->get( 'SessionCacheType' ) );
+                       $this->permStore->setLogger( $this->logger );
                }
 
                register_shutdown_function( array( $this, 'shutdown' ) );
@@ -202,7 +206,14 @@ final class SessionManager implements SessionManagerInterface {
                // Test this here to provide a better log message for the common case
                // of "no such ID"
                $key = wfMemcKey( 'MWSession', $id );
-               if ( is_array( $this->store->get( $key ) ) ) {
+               $existing = $this->tempStore->get( $key );
+               if ( $existing === false ) {
+                       $existing = $this->permStore->get( $key );
+                       if ( $existing !== false ) {
+                               $this->tempStore->set( $key, $existing );
+                       }
+               }
+               if ( is_array( $existing ) ) {
                        $info = new SessionInfo( SessionInfo::MIN_PRIORITY, array( 'id' => $id, 'idIsSafe' => true ) );
                        if ( $this->loadSessionInfoFromStore( $info, $request ) ) {
                                $session = $this->getSessionFromInfo( $info, $request );
@@ -240,7 +251,14 @@ final class SessionManager implements SessionManagerInterface {
                        }
 
                        $key = wfMemcKey( 'MWSession', $id );
-                       if ( is_array( $this->store->get( $key ) ) ) {
+                       $existing = $this->tempStore->get( $key );
+                       if ( $existing === false ) {
+                               $existing = $this->permStore->get( $key );
+                               if ( $existing !== false ) {
+                                       $this->tempStore->set( $key, $existing );
+                               }
+                       }
+                       if ( is_array( $existing ) ) {
                                throw new \InvalidArgumentException( 'Session ID already exists' );
                        }
                }
@@ -660,7 +678,13 @@ final class SessionManager implements SessionManagerInterface {
         */
        private function loadSessionInfoFromStore( SessionInfo &$info, WebRequest $request ) {
                $key = wfMemcKey( 'MWSession', $info->getId() );
-               $blob = $this->store->get( $key );
+               $blob = $this->tempStore->get( $key );
+               if ( $blob === false ) {
+                       $blob = $this->permStore->get( $key );
+                       if ( $blob !== false ) {
+                               $this->tempStore->set( $key, $blob );
+                       }
+               }
 
                $newParams = array();
 
@@ -668,7 +692,8 @@ final class SessionManager implements SessionManagerInterface {
                        // Sanity check: blob must be an array, if it's saved at all
                        if ( !is_array( $blob ) ) {
                                $this->logger->warning( "Session $info: Bad data" );
-                               $this->store->delete( $key );
+                               $this->tempStore->delete( $key );
+                               $this->permStore->delete( $key );
                                return false;
                        }
 
@@ -677,7 +702,8 @@ final class SessionManager implements SessionManagerInterface {
                                !isset( $blob['metadata'] ) || !is_array( $blob['metadata'] )
                        ) {
                                $this->logger->warning( "Session $info: Bad data structure" );
-                               $this->store->delete( $key );
+                               $this->tempStore->delete( $key );
+                               $this->permStore->delete( $key );
                                return false;
                        }
 
@@ -692,7 +718,8 @@ final class SessionManager implements SessionManagerInterface {
                                !array_key_exists( 'provider', $metadata )
                        ) {
                                $this->logger->warning( "Session $info: Bad metadata" );
-                               $this->store->delete( $key );
+                               $this->tempStore->delete( $key );
+                               $this->permStore->delete( $key );
                                return false;
                        }
 
@@ -702,7 +729,8 @@ final class SessionManager implements SessionManagerInterface {
                                $newParams['provider'] = $provider = $this->getProvider( $metadata['provider'] );
                                if ( !$provider ) {
                                        $this->logger->warning( "Session $info: Unknown provider, " . $metadata['provider'] );
-                                       $this->store->delete( $key );
+                                       $this->tempStore->delete( $key );
+                                       $this->permStore->delete( $key );
                                        return false;
                                }
                        } elseif ( $metadata['provider'] !== (string)$provider ) {
@@ -893,7 +921,8 @@ final class SessionManager implements SessionManagerInterface {
                        $backend = new SessionBackend(
                                $this->allSessionIds[$id],
                                $info,
-                               $this->store,
+                               $this->tempStore,
+                               $this->permStore,
                                $this->logger,
                                $this->config->get( 'ObjectCacheSessionExpiry' )
                        );
@@ -970,7 +999,9 @@ final class SessionManager implements SessionManagerInterface {
                do {
                        $id = wfBaseConvert( \MWCryptRand::generateHex( 40 ), 16, 32, 32 );
                        $key = wfMemcKey( 'MWSession', $id );
-               } while ( isset( $this->allSessionIds[$id] ) || is_array( $this->store->get( $key ) ) );
+               } while ( isset( $this->allSessionIds[$id] ) ||
+                       is_array( $this->tempStore->get( $key ) ) || is_array( $this->permStore->get( $key ) )
+               );
                return $id;
        }
 
@@ -980,7 +1011,7 @@ final class SessionManager implements SessionManagerInterface {
         * @param PHPSessionHandler $handler
         */
        public function setupPHPSessionHandler( PHPSessionHandler $handler ) {
-               $handler->setManager( $this, $this->store, $this->logger );
+               $handler->setManager( $this, $this->permStore, $this->logger );
        }
 
        /**

diff --git a/tests/phpunit/includes/session/CookieSessionProviderTest.php b/tests/phpunit/includes/session/CookieSessionProviderTest.php
index 702f556..2b7e0a1 100644 (file)
--- a/tests/phpunit/includes/session/CookieSessionProviderTest.php
+++ b/tests/phpunit/includes/session/CookieSessionProviderTest.php
@@ -363,6 +363,7 @@ class CookieSessionProviderTest extends MediaWikiTestCase {
                                'idIsSafe' => true,
                        ) ),
                        $store,
+                       $store,
                        new \Psr\Log\NullLogger(),
                        10
                );
@@ -449,6 +450,7 @@ class CookieSessionProviderTest extends MediaWikiTestCase {
                                'idIsSafe' => true,
                        ) ),
                        new \EmptyBagOStuff(),
+                       new \EmptyBagOStuff(),
                        new \Psr\Log\NullLogger(),
                        10
                );
@@ -553,6 +555,7 @@ class CookieSessionProviderTest extends MediaWikiTestCase {
                                'idIsSafe' => true,
                        ) ),
                        $store,
+                       $store,
                        new \Psr\Log\NullLogger(),
                        10
                );

diff --git a/tests/phpunit/includes/session/ImmutableSessionProviderWithCookieTest.php b/tests/phpunit/includes/session/ImmutableSessionProviderWithCookieTest.php
index e06dfd5..46f23f3 100644 (file)
--- a/tests/phpunit/includes/session/ImmutableSessionProviderWithCookieTest.php
+++ b/tests/phpunit/includes/session/ImmutableSessionProviderWithCookieTest.php
@@ -205,6 +205,7 @@ class ImmutableSessionProviderWithCookieTest extends MediaWikiTestCase {
                                'idIsSafe' => true,
                        ) ),
                        new \EmptyBagOStuff(),
+                       new \EmptyBagOStuff(),
                        new \Psr\Log\NullLogger(),
                        10
                );

diff --git a/tests/phpunit/includes/session/PHPSessionHandlerTest.php b/tests/phpunit/includes/session/PHPSessionHandlerTest.php
index 125e1b6..1c54a20 100644 (file)
--- a/tests/phpunit/includes/session/PHPSessionHandlerTest.php
+++ b/tests/phpunit/includes/session/PHPSessionHandlerTest.php
@@ -172,14 +172,6 @@ class PHPSessionHandlerTest extends MediaWikiTestCase {
                        $this->assertSame( $expect, $_SESSION );
                }
 
-               // Test expiry
-               session_write_close();
-               ini_set( 'session.gc_divisor', 1 );
-               ini_set( 'session.gc_probability', 1 );
-               sleep( 3 );
-               session_start();
-               $this->assertSame( array(), $_SESSION );
-
                // Re-fill the session, then test that session_destroy() works.
                $_SESSION['AuthenticationSessionTest'] = $rand;
                session_write_close();

diff --git a/tests/phpunit/includes/session/SessionBackendTest.php b/tests/phpunit/includes/session/SessionBackendTest.php
index d06706b..85fa9bd 100644 (file)
--- a/tests/phpunit/includes/session/SessionBackendTest.php
+++ b/tests/phpunit/includes/session/SessionBackendTest.php
@@ -59,7 +59,7 @@ class SessionBackendTest extends MediaWikiTestCase {
                ) );
                $id = new SessionId( $info->getId() );
 
-               $backend = new SessionBackend( $id, $info, $this->store, $logger, 10 );
+               $backend = new SessionBackend( $id, $info, $this->store, $this->store, $logger, 10 );
                $priv = \TestingAccessWrapper::newFromObject( $backend );
                $priv->persist = false;
                $priv->requests = array( 100 => new \FauxRequest() );
@@ -87,7 +87,7 @@ class SessionBackendTest extends MediaWikiTestCase {
                $id = new SessionId( $info->getId() );
                $logger = new \Psr\Log\NullLogger();
                try {
-                       new SessionBackend( $id, $info, $this->store, $logger, 10 );
+                       new SessionBackend( $id, $info, $this->store, $this->store, $logger, 10 );
                        $this->fail( 'Expected exception not thrown' );
                } catch ( \InvalidArgumentException $ex ) {
                        $this->assertSame(
@@ -103,7 +103,7 @@ class SessionBackendTest extends MediaWikiTestCase {
                ) );
                $id = new SessionId( $info->getId() );
                try {
-                       new SessionBackend( $id, $info, $this->store, $logger, 10 );
+                       new SessionBackend( $id, $info, $this->store, $this->store, $logger, 10 );
                        $this->fail( 'Expected exception not thrown' );
                } catch ( \InvalidArgumentException $ex ) {
                        $this->assertSame( 'Cannot create session without a provider', $ex->getMessage() );
@@ -118,7 +118,7 @@ class SessionBackendTest extends MediaWikiTestCase {
                ) );
                $id = new SessionId( '!' . $info->getId() );
                try {
-                       new SessionBackend( $id, $info, $this->store, $logger, 10 );
+                       new SessionBackend( $id, $info, $this->store, $this->store, $logger, 10 );
                        $this->fail( 'Expected exception not thrown' );
                } catch ( \InvalidArgumentException $ex ) {
                        $this->assertSame(
@@ -135,7 +135,7 @@ class SessionBackendTest extends MediaWikiTestCase {
                        'idIsSafe' => true,
                ) );
                $id = new SessionId( $info->getId() );
-               $backend = new SessionBackend( $id, $info, $this->store, $logger, 10 );
+               $backend = new SessionBackend( $id, $info, $this->store, $this->store, $logger, 10 );
                $this->assertSame( self::SESSIONID, $backend->getId() );
                $this->assertSame( $id, $backend->getSessionId() );
                $this->assertSame( $this->provider, $backend->getProvider() );
@@ -157,7 +157,7 @@ class SessionBackendTest extends MediaWikiTestCase {
                        'idIsSafe' => true,
                ) );
                $id = new SessionId( $info->getId() );
-               $backend = new SessionBackend( $id, $info, $this->store, $logger, 10 );
+               $backend = new SessionBackend( $id, $info, $this->store, $this->store, $logger, 10 );
                $this->assertSame( self::SESSIONID, $backend->getId() );
                $this->assertSame( $id, $backend->getSessionId() );
                $this->assertSame( $this->provider, $backend->getProvider() );

diff --git a/tests/phpunit/includes/session/SessionManagerTest.php b/tests/phpunit/includes/session/SessionManagerTest.php
index f5bb07d..4fde341 100644 (file)
--- a/tests/phpunit/includes/session/SessionManagerTest.php
+++ b/tests/phpunit/includes/session/SessionManagerTest.php
@@ -103,7 +103,7 @@ class SessionManagerTest extends MediaWikiTestCase {
                $manager = \TestingAccessWrapper::newFromObject( $this->getManager() );
                $this->assertSame( $this->config, $manager->config );
                $this->assertSame( $this->logger, $manager->logger );
-               $this->assertSame( $this->store, $manager->store );
+               $this->assertSame( $this->store, $manager->permStore );
 
                $manager = \TestingAccessWrapper::newFromObject( new SessionManager() );
                $this->assertSame( \RequestContext::getMain()->getConfig(), $manager->config );
@@ -111,7 +111,7 @@ class SessionManagerTest extends MediaWikiTestCase {
                $manager = \TestingAccessWrapper::newFromObject( new SessionManager( array(
                        'config' => $this->config,
                ) ) );
-               $this->assertSame( \ObjectCache::$instances['testSessionStore'], $manager->store );
+               $this->assertSame( \ObjectCache::$instances['testSessionStore'], $manager->permStore );
 
                foreach ( array(
                        'config' => '$options[\'config\'] must be an instance of Config',
@@ -301,6 +301,9 @@ class SessionManagerTest extends MediaWikiTestCase {
        public function testGetSessionById() {
                $manager = $this->getManager();
 
+               // Disable the in-process cache so our $this->store->setSession() takes effect.
+               \TestingAccessWrapper::newFromObject( $manager )->tempStore = new \EmptyBagOStuff;
+
                try {
                        $manager->getSessionById( 'bad' );
                        $this->fail( 'Expected exception not thrown' );
@@ -1083,6 +1086,9 @@ class SessionManagerTest extends MediaWikiTestCase {
                $manager->setLogger( $logger );
                $request = new \FauxRequest();
 
+               // Disable the in-process cache so our $this->store->setSession() takes effect.
+               \TestingAccessWrapper::newFromObject( $manager )->tempStore = new \EmptyBagOStuff;
+
                // TestingAccessWrapper can't handle methods with reference arguments, sigh.
                $rClass = new \ReflectionClass( $manager );
                $rMethod = $rClass->getMethod( 'loadSessionInfoFromStore' );