From fad75f939de59d35930470a9f326c14c07806f37 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 27 Apr 2015 10:25:53 -0700
Subject: [PATCH] Improve messaging around repository locks

Summary:
Fixes T6958. Ref T7484.

  - When we collide on a lock in `bin/repository update`, explain what that means.
  - GlobalLock currently uses a "lock name" which is different from the lock's actual name. Don't do this. There's a small chance this fixes T7484, but I don't have high hopes.

Test Plan: Ran `bin/repository update X` in two windows really fast, got the new message in one of them.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T6958, T7484

Differential Revision: https://secure.phabricator.com/D12574
---
 ...ricatorRepositoryManagementUpdateWorkflow.php | 16 +++++++++++++++-
 .../util/PhabricatorGlobalLock.php               |  8 +++-----
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/src/applications/repository/management/PhabricatorRepositoryManagementUpdateWorkflow.php b/src/applications/repository/management/PhabricatorRepositoryManagementUpdateWorkflow.php
index a664362d91..afe8b9e5d5 100644
--- a/src/applications/repository/management/PhabricatorRepositoryManagementUpdateWorkflow.php
+++ b/src/applications/repository/management/PhabricatorRepositoryManagementUpdateWorkflow.php
@@ -56,7 +56,21 @@ final class PhabricatorRepositoryManagementUpdateWorkflow
       $lock_name = 'repository.update:'.$repository->getID();
       $lock = PhabricatorGlobalLock::newLock($lock_name);
 
-      $lock->lock();
+      try {
+        $lock->lock();
+      } catch (PhutilLockException $ex) {
+        throw new PhutilProxyException(
+          pht(
+            'Another process is currently holding the update lock for '.
+            'repository "%s". Repositories may only be updated by one '.
+            'process at a time. This can happen if you are running multiple '.
+            'copies of the daemons. This can also happen if you manually '.
+            'update a repository while the daemons are also updating it '.
+            '(in this case, just try again in a few moments).',
+            $repository->getMonogram()),
+          $ex);
+      }
+
       try {
         $no_discovery = $args->getArg('no-discovery');
 
diff --git a/src/infrastructure/util/PhabricatorGlobalLock.php b/src/infrastructure/util/PhabricatorGlobalLock.php
index eca6ee4228..41558531d7 100644
--- a/src/infrastructure/util/PhabricatorGlobalLock.php
+++ b/src/infrastructure/util/PhabricatorGlobalLock.php
@@ -28,7 +28,6 @@
  */
 final class PhabricatorGlobalLock extends PhutilLock {
 
-  private $lockname;
   private $conn;
 
   private static $pool = array();
@@ -41,7 +40,7 @@ final class PhabricatorGlobalLock extends PhutilLock {
     $namespace = PhabricatorLiskDAO::getStorageNamespace();
     $namespace = PhabricatorHash::digestToLength($namespace, 20);
 
-    $full_name = $namespace.'-g:'.$name;
+    $full_name = 'ph:'.$namespace.':'.$name;
 
     $length_limit = 64;
     if (strlen($full_name) > $length_limit) {
@@ -57,7 +56,6 @@ final class PhabricatorGlobalLock extends PhutilLock {
     $lock = self::getLock($full_name);
     if (!$lock) {
       $lock = new PhabricatorGlobalLock($full_name);
-      $lock->lockname = $name;
       self::registerLock($lock);
     }
 
@@ -99,7 +97,7 @@ final class PhabricatorGlobalLock extends PhutilLock {
     $result = queryfx_one(
       $conn,
       'SELECT GET_LOCK(%s, %f)',
-      'phabricator:'.$this->lockname,
+      $this->getName(),
       $wait);
 
     $ok = head($result);
@@ -114,7 +112,7 @@ final class PhabricatorGlobalLock extends PhutilLock {
     queryfx(
       $this->conn,
       'SELECT RELEASE_LOCK(%s)',
-      'phabricator:'.$this->lockname);
+      $this->getName());
 
     $this->conn->close();
     self::$pool[] = $this->conn;