From 12cac7cca9ecba19c6ffa8adf992f3de58e76af6 Mon Sep 17 00:00:00 2001 From: Tim Starling Date: Mon, 1 Nov 2021 13:40:54 +1100 Subject: [PATCH] LockManager: distinguish conflicts from other kinds of lock errors With a user-friendly error message which uses both technical and non-technical language, without the irrelevant detail of filebackend URLs. Bug: T283045 Change-Id: I8704f405d38acdffae7bd4ea5b91f3f591fc2ebc --- .../filebackend/lockmanager/MySqlLockManager.php | 4 +--- includes/libs/lockmanager/FSLockManager.php | 2 +- includes/libs/lockmanager/MemcLockManager.php | 9 +++------ includes/libs/lockmanager/PostgreSqlLockManager.php | 4 +--- includes/libs/lockmanager/RedisLockManager.php | 6 ++---- languages/i18n/en.json | 1 + languages/i18n/qqq.json | 1 + .../filebackend/lockmanager/MySqlLockManagerTest.php | 12 +++++------- 8 files changed, 15 insertions(+), 24 deletions(-) diff --git a/includes/filebackend/lockmanager/MySqlLockManager.php b/includes/filebackend/lockmanager/MySqlLockManager.php index 9bee0383354..ec5eb6390e1 100644 --- a/includes/filebackend/lockmanager/MySqlLockManager.php +++ b/includes/filebackend/lockmanager/MySqlLockManager.php @@ -105,9 +105,7 @@ class MySqlLockManager extends DBLockManager { } if ( $blocked ) { - foreach ( $paths as $path ) { - $status->fatal( 'lockmanager-fail-acquirelock', $path ); - } + $status->fatal( 'lockmanager-fail-conflict' ); } return $status; diff --git a/includes/libs/lockmanager/FSLockManager.php b/includes/libs/lockmanager/FSLockManager.php index a901e1049db..dc99d5d7381 100644 --- a/includes/libs/lockmanager/FSLockManager.php +++ b/includes/libs/lockmanager/FSLockManager.php @@ -143,7 +143,7 @@ class FSLockManager extends LockManager { $this->handles[$path] = $handle; } else { fclose( $handle ); - $status->fatal( 'lockmanager-fail-acquirelock', $path ); + $status->fatal( 'lockmanager-fail-conflict' ); } } else { $status->fatal( 'lockmanager-fail-openlock', $path ); diff --git a/includes/libs/lockmanager/MemcLockManager.php b/includes/libs/lockmanager/MemcLockManager.php index 47c29aefddb..5eeebd3e67c 100644 --- a/includes/libs/lockmanager/MemcLockManager.php +++ b/includes/libs/lockmanager/MemcLockManager.php @@ -101,10 +101,7 @@ class MemcLockManager extends QuorumLockManager { // Lock all of the active lock record keys... if ( !$this->acquireMutexes( $memc, $keys ) ) { - foreach ( $paths as $path ) { - $status->fatal( 'lockmanager-fail-acquirelock', $path ); - } - + $status->fatal( 'lockmanager-fail-conflict' ); return $status; } @@ -123,7 +120,7 @@ class MemcLockManager extends QuorumLockManager { if ( $expiry < $now ) { // stale? unset( $locksHeld[self::LOCK_EX][$session] ); } elseif ( $session !== $this->session ) { - $status->fatal( 'lockmanager-fail-acquirelock', $path ); + $status->fatal( 'lockmanager-fail-conflict' ); } } if ( $type === self::LOCK_EX ) { @@ -131,7 +128,7 @@ class MemcLockManager extends QuorumLockManager { if ( $expiry < $now ) { // stale? unset( $locksHeld[self::LOCK_SH][$session] ); } elseif ( $session !== $this->session ) { - $status->fatal( 'lockmanager-fail-acquirelock', $path ); + $status->fatal( 'lockmanager-fail-conflict' ); } } } diff --git a/includes/libs/lockmanager/PostgreSqlLockManager.php b/includes/libs/lockmanager/PostgreSqlLockManager.php index 5f63334726c..4b8575c84a4 100644 --- a/includes/libs/lockmanager/PostgreSqlLockManager.php +++ b/includes/libs/lockmanager/PostgreSqlLockManager.php @@ -54,9 +54,7 @@ class PostgreSqlLockManager extends DBLockManager { if ( count( $fields ) ) { $db->query( 'SELECT ' . implode( ', ', $fields ), __METHOD__ ); } - foreach ( $paths as $path ) { - $status->fatal( 'lockmanager-fail-acquirelock', $path ); - } + $status->fatal( 'lockmanager-fail-conflict' ); } return $status; diff --git a/includes/libs/lockmanager/RedisLockManager.php b/includes/libs/lockmanager/RedisLockManager.php index 3c84f419ec7..0c94ff86173 100644 --- a/includes/libs/lockmanager/RedisLockManager.php +++ b/includes/libs/lockmanager/RedisLockManager.php @@ -164,10 +164,8 @@ LUA; foreach ( $pathList as $path ) { $status->fatal( 'lockmanager-fail-acquirelock', $path ); } - } else { - foreach ( $res as $key ) { - $status->fatal( 'lockmanager-fail-acquirelock', $pathsByKey[$key] ); - } + } elseif ( count( $res ) ) { + $status->fatal( 'lockmanager-fail-conflict' ); } return $status; diff --git a/languages/i18n/en.json b/languages/i18n/en.json index 2d2f9448d34..c422a3f4ccb 100644 --- a/languages/i18n/en.json +++ b/languages/i18n/en.json @@ -1870,6 +1870,7 @@ "lockmanager-fail-db-release": "Could not release locks on database $1.", "lockmanager-fail-svr-acquire": "Could not acquire locks on server $1.", "lockmanager-fail-svr-release": "Could not release locks on server $1.", + "lockmanager-fail-conflict": "Could not acquire lock. Somebody else is doing something to this file.", "zip-file-open-error": "An error was encountered when opening the file for ZIP checks.", "zip-wrong-format": "The specified file was not a ZIP file.", "zip-bad": "The file is a corrupt or otherwise unreadable ZIP file.\nIt cannot be properly checked for security.", diff --git a/languages/i18n/qqq.json b/languages/i18n/qqq.json index 3681bd52bd3..9a2c374572f 100644 --- a/languages/i18n/qqq.json +++ b/languages/i18n/qqq.json @@ -2097,6 +2097,7 @@ "lockmanager-fail-db-release": "Parameters:\n* $1 is a database name.", "lockmanager-fail-svr-acquire": "Parameters:\n* $1 - server", "lockmanager-fail-svr-release": "Parameters:\n* $1 is a server name.", + "lockmanager-fail-conflict": "Used as an error message when the lock is held by another process.", "zip-file-open-error": "Used as ZIP error message.\n\nSee also:\n* {{msg-mw|Zip-wrong-format}}\n* {{msg-mw|Zip-bad}}\n* {{msg-mw|Zip-unsupported}}", "zip-wrong-format": "Used as ZIP error message.\n\nSee also:\n* {{msg-mw|Zip-file-open-error}}\n* {{msg-mw|Zip-bad}}\n* {{msg-mw|Zip-unsupported}}", "zip-bad": "Used as ZIP error message.\n\nSee also:\n* {{msg-mw|Zip-file-open-error}}\n* {{msg-mw|Zip-wrong-format}}\n* {{msg-mw|Zip-unsupported}}", diff --git a/tests/phpunit/unit/includes/filebackend/lockmanager/MySqlLockManagerTest.php b/tests/phpunit/unit/includes/filebackend/lockmanager/MySqlLockManagerTest.php index db2e4e903ec..aebbc755ef2 100644 --- a/tests/phpunit/unit/includes/filebackend/lockmanager/MySqlLockManagerTest.php +++ b/tests/phpunit/unit/includes/filebackend/lockmanager/MySqlLockManagerTest.php @@ -139,13 +139,11 @@ class MySqlLockManagerTest extends MediaWikiUnitTestCase { $expectedErrors = []; if ( !( $params['expectedOK'] ?? true ) ) { - foreach ( $params['lockArgs'][0] as $path ) { - $expectedErrors[] = [ - 'type' => 'error', - 'message' => 'lockmanager-fail-acquirelock', - 'params' => [ $path ], - ]; - } + $expectedErrors[] = [ + 'type' => 'error', + 'message' => 'lockmanager-fail-conflict', + 'params' => [], + ]; } $this->assertSame( $expectedErrors, $status->getErrors() ); $this->assertSame( !$expectedErrors, $status->isOK() );