Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid locking the php session #32162

Merged
merged 3 commits into from
Aug 23, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions config/config.sample.php
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,17 @@
*/
'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.
Expand Down
17 changes: 13 additions & 4 deletions lib/base.php
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,9 @@ public static function initSession() {
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)) {
Expand All @@ -455,7 +457,10 @@ public static function initSession() {
\OC::$server->getUserSession()->logout();
}

$session->set('LAST_ACTIVITY', time());
if (!self::hasSessionRelaxedExpiry()) {
$session->set('LAST_ACTIVITY', time());
}
$session->close();
}

/**
Expand All @@ -465,6 +470,13 @@ private static function getSessionLifeTime() {
return \OC::$server->getConfig()->getSystemValue('session_lifetime', 60 * 60 * 24);
}

/**
* @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
*/
Expand Down Expand Up @@ -707,9 +719,6 @@ public static function init() {
$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)) {
Expand Down
4 changes: 2 additions & 2 deletions lib/private/AppFramework/Middleware/SessionMiddleware.php
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@ public function __construct(ControllerMethodReflector $reflector,
*/
public function beforeController($controller, $methodName) {
$useSession = $this->reflector->hasAnnotation('UseSession');
if (!$useSession) {
$this->session->close();
if ($useSession) {
$this->session->reopen();
}
}

Expand Down
17 changes: 17 additions & 0 deletions lib/private/Session/CryptoSessionData.php
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,17 @@ protected function initializeSession() {
* @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();
}
}

/**
Expand Down Expand Up @@ -131,9 +140,13 @@ public function exists(string $key): bool {
* @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();
}
}

/**
Expand All @@ -149,6 +162,10 @@ public function clear() {
$this->session->clear();
}

public function reopen(): bool {
return $this->session->reopen();
}

/**
* Wrapper around session_regenerate_id
*
Expand Down
25 changes: 20 additions & 5 deletions lib/private/Session/Internal.php
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,11 @@ public function __construct(string $name) {
* @param integer $value
*/
public function set(string $key, $value) {
$this->validateSession();
$reopened = $this->reopen();
$_SESSION[$key] = $value;
if ($reopened) {
$this->close();
}
}

/**
Expand Down Expand Up @@ -101,6 +104,7 @@ public function remove(string $key) {
}

public function clear() {
$this->reopen();
$this->invoke('session_unset');
$this->regenerateId();
$this->startSession(true);
Expand All @@ -120,6 +124,7 @@ public function close() {
* @return void
*/
public function regenerateId(bool $deleteOldSession = true, bool $updateToken = false) {
$this->reopen();
$oldId = null;

if ($updateToken) {
Expand Down Expand Up @@ -171,8 +176,14 @@ public function getId(): string {
/**
* @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;
}

/**
Expand Down Expand Up @@ -214,7 +225,11 @@ private function invoke(string $functionName, array $parameters = [], bool $sile
}
}

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);
}
}
6 changes: 3 additions & 3 deletions lib/private/Session/Memory.php
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ public function __construct(string $name) {
* @param integer $value
*/
public function set(string $key, $value) {
$this->validateSession();
$this->data[$key] = $value;
}

Expand All @@ -80,7 +79,6 @@ public function exists(string $key): bool {
* @param string $key
*/
public function remove(string $key) {
$this->validateSession();
unset($this->data[$key]);
}

Expand Down Expand Up @@ -110,8 +108,10 @@ public function getId(): string {
/**
* Helper function for PHPUnit execution - don't use in non-test code
*/
public function reopen() {
public function reopen(): bool {
Fixed Show fixed Hide fixed
$reopened = $this->sessionClosed;
$this->sessionClosed = false;
return $reopened;
}

/**
Expand Down
8 changes: 8 additions & 0 deletions lib/public/ISession.php
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,14 @@ public function remove(string $key);
*/
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
Expand Down