Merge pull request #32162 from nextcloud/enh/session-reopen

Avoid locking the php session

7 files changed, 74 insertions, 14 deletions

diff --git a/config/config.sample.php b/config/config.sample.php
index 025cf1105a0..fe45223361f 100644
--- a/config/config.sample.php
+++ b/config/config.sample.php
@@ -257,6 +257,17 @@ $CONFIG = [
 'session_lifetime' => 60 * 60 * 24,
 
 /**
+ * `true` enabled a relaxed session timeout, where the session timeout would no longer be
+ * handled by Nextcloud but by either the PHP garbage collection or the expiration of
+ * potential other session backends like redis.
+ *
+ * This may lead to sessions being available for longer than what session_lifetime uses but
+ * comes with performance benefits as sessions are no longer a locking operation for concurrent
+ * requests.
+ */
+'session_relaxed_expiry' => false,
+
+/**
  * Enable or disable session keep-alive when a user is logged in to the Web UI.
  * Enabling this sends a "heartbeat" to the server to keep it from timing out.
  *
diff --git a/lib/base.php b/lib/base.php
index 54b7a5e3629..1fca124f072 100644
--- a/lib/base.php
+++ b/lib/base.php
@@ -445,7 +445,9 @@ class OC {
 			die();
 		}
 
+		//try to set the session lifetime
 		$sessionLifeTime = self::getSessionLifeTime();
+		@ini_set('gc_maxlifetime', (string)$sessionLifeTime);
 
 		// session timeout
 		if ($session->exists('LAST_ACTIVITY') && (time() - $session->get('LAST_ACTIVITY') > $sessionLifeTime)) {
@@ -455,7 +457,10 @@ class OC {
 			\OC::$server->getUserSession()->logout();
 		}
 
-		$session->set('LAST_ACTIVITY', time());
+		if (!self::hasSessionRelaxedExpiry()) {
+			$session->set('LAST_ACTIVITY', time());
+		}
+		$session->close();
 	}
 
 	/**
@@ -466,6 +471,13 @@ class OC {
 	}
 
 	/**
+	 * @return bool true if the session expiry should only be done by gc instead of an explicit timeout
+	 */
+	public static function hasSessionRelaxedExpiry(): bool {
+		return \OC::$server->getConfig()->getSystemValue('session_relaxed_expiry', false);
+	}
+
+	/**
 	 * Try to set some values to the required Nextcloud default
 	 */
 	public static function setRequiredIniValues() {
@@ -707,9 +719,6 @@ class OC {
 				$config->deleteAppValue('core', 'cronErrors');
 			}
 		}
-		//try to set the session lifetime
-		$sessionLifeTime = self::getSessionLifeTime();
-		@ini_set('gc_maxlifetime', (string)$sessionLifeTime);
 
 		// User and Groups
 		if (!$systemConfig->getValue("installed", false)) {
diff --git a/lib/private/AppFramework/Middleware/SessionMiddleware.php b/lib/private/AppFramework/Middleware/SessionMiddleware.php
index f3fd2c99173..32ac2b17ae5 100644
--- a/lib/private/AppFramework/Middleware/SessionMiddleware.php
+++ b/lib/private/AppFramework/Middleware/SessionMiddleware.php
@@ -51,8 +51,8 @@ class SessionMiddleware extends Middleware {
 	 */
 	public function beforeController($controller, $methodName) {
 		$useSession = $this->reflector->hasAnnotation('UseSession');
-		if (!$useSession) {
-			$this->session->close();
+		if ($useSession) {
+			$this->session->reopen();
 		}
 	}
 
diff --git a/lib/private/Session/CryptoSessionData.php b/lib/private/Session/CryptoSessionData.php
index 2e3bd46da5b..b01887e39e2 100644
--- a/lib/private/Session/CryptoSessionData.php
+++ b/lib/private/Session/CryptoSessionData.php
@@ -97,8 +97,17 @@ class CryptoSessionData implements \ArrayAccess, ISession {
 	 * @param mixed $value
 	 */
 	public function set(string $key, $value) {
+		if ($this->get($key) === $value) {
+			// Do not write the session if the value hasn't changed to avoid reopening
+			return;
+		}
+
+		$reopened = $this->reopen();
 		$this->sessionValues[$key] = $value;
 		$this->isModified = true;
+		if ($reopened) {
+			$this->close();
+		}
 	}
 
 	/**
@@ -131,9 +140,13 @@ class CryptoSessionData implements \ArrayAccess, ISession {
 	 * @param string $key
 	 */
 	public function remove(string $key) {
+		$reopened = $this->reopen();
 		$this->isModified = true;
 		unset($this->sessionValues[$key]);
 		$this->session->remove(self::encryptedSessionName);
+		if ($reopened) {
+			$this->close();
+		}
 	}
 
 	/**
@@ -149,6 +162,10 @@ class CryptoSessionData implements \ArrayAccess, ISession {
 		$this->session->clear();
 	}
 
+	public function reopen(): bool {
+		return $this->session->reopen();
+	}
+
 	/**
 	 * Wrapper around session_regenerate_id
 	 *
diff --git a/lib/private/Session/Internal.php b/lib/private/Session/Internal.php
index 6e0c54c6fab..87dd5ed6014 100644
--- a/lib/private/Session/Internal.php
+++ b/lib/private/Session/Internal.php
@@ -68,8 +68,11 @@ class Internal extends Session {
 	 * @param integer $value
 	 */
 	public function set(string $key, $value) {
-		$this->validateSession();
+		$reopened = $this->reopen();
 		$_SESSION[$key] = $value;
+		if ($reopened) {
+			$this->close();
+		}
 	}
 
 	/**
@@ -101,6 +104,7 @@ class Internal extends Session {
 	}
 
 	public function clear() {
+		$this->reopen();
 		$this->invoke('session_unset');
 		$this->regenerateId();
 		$this->startSession(true);
@@ -120,6 +124,7 @@ class Internal extends Session {
 	 * @return void
 	 */
 	public function regenerateId(bool $deleteOldSession = true, bool $updateToken = false) {
+		$this->reopen();
 		$oldId = null;
 
 		if ($updateToken) {
@@ -171,8 +176,14 @@ class Internal extends Session {
 	/**
 	 * @throws \Exception
 	 */
-	public function reopen() {
-		throw new \Exception('The session cannot be reopened - reopen() is only to be used in unit testing.');
+	public function reopen(): bool {
+		if ($this->sessionClosed) {
+			$this->startSession(false, false);
+			$this->sessionClosed = false;
+			return true;
+		}
+
+		return false;
 	}
 
 	/**
@@ -214,7 +225,11 @@ class Internal extends Session {
 		}
 	}
 
-	private function startSession(bool $silence = false) {
-		$this->invoke('session_start', [['cookie_samesite' => 'Lax']], $silence);
+	private function startSession(bool $silence = false, bool $readAndClose = true) {
+		$sessionParams = ['cookie_samesite' => 'Lax'];
+		if (\OC::hasSessionRelaxedExpiry()) {
+			$sessionParams['read_and_close'] = $readAndClose;
+		}
+		$this->invoke('session_start', [$sessionParams], $silence);
 	}
 }
diff --git a/lib/private/Session/Memory.php b/lib/private/Session/Memory.php
index 0afd3703366..b9b3dba54b7 100644
--- a/lib/private/Session/Memory.php
+++ b/lib/private/Session/Memory.php
@@ -53,7 +53,6 @@ class Memory extends Session {
 	 * @param integer $value
 	 */
 	public function set(string $key, $value) {
-		$this->validateSession();
 		$this->data[$key] = $value;
 	}
 
@@ -80,7 +79,6 @@ class Memory extends Session {
 	 * @param string $key
 	 */
 	public function remove(string $key) {
-		$this->validateSession();
 		unset($this->data[$key]);
 	}
 
@@ -110,8 +108,10 @@ class Memory extends Session {
 	/**
 	 * Helper function for PHPUnit execution - don't use in non-test code
 	 */
-	public function reopen() {
+	public function reopen(): bool {
+		$reopened = $this->sessionClosed;
 		$this->sessionClosed = false;
+		return $reopened;
 	}
 
 	/**
diff --git a/lib/public/ISession.php b/lib/public/ISession.php
index 2709e09d4ca..12cd716ec3f 100644
--- a/lib/public/ISession.php
+++ b/lib/public/ISession.php
@@ -84,6 +84,14 @@ interface ISession {
 	public function clear();
 
 	/**
+	 * Reopen a session for writing again
+	 *
+	 * @return bool true if the session was actually reopened, otherwise false
+	 * @since 25.0.0
+	 */
+	public function reopen(): bool;
+
+	/**
 	 * Close the session and release the lock
 	 * @since 7.0.0
 	 */