Page MenuHomePhabricator
Files F15293079

D19080.diff
No OneTemporary
Actions

D19080.diff
View Options

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -1113,6 +1113,7 @@
'DrydockResourceDatasource' => 'applications/drydock/typeahead/DrydockResourceDatasource.php',
'DrydockResourceListController' => 'applications/drydock/controller/DrydockResourceListController.php',
'DrydockResourceListView' => 'applications/drydock/view/DrydockResourceListView.php',
+ 'DrydockResourceLockException' => 'applications/drydock/exception/DrydockResourceLockException.php',
'DrydockResourcePHIDType' => 'applications/drydock/phid/DrydockResourcePHIDType.php',
'DrydockResourceQuery' => 'applications/drydock/query/DrydockResourceQuery.php',
'DrydockResourceReclaimLogType' => 'applications/drydock/logtype/DrydockResourceReclaimLogType.php',
@@ -6344,6 +6345,7 @@
'DrydockResourceDatasource' => 'PhabricatorTypeaheadDatasource',
'DrydockResourceListController' => 'DrydockResourceController',
'DrydockResourceListView' => 'AphrontView',
+ 'DrydockResourceLockException' => 'Exception',
'DrydockResourcePHIDType' => 'PhabricatorPHIDType',
'DrydockResourceQuery' => 'DrydockQuery',
'DrydockResourceReclaimLogType' => 'DrydockLogType',
diff --git a/src/applications/drydock/exception/DrydockResourceLockException.php b/src/applications/drydock/exception/DrydockResourceLockException.php
new file mode 100644
--- /dev/null
+++ b/src/applications/drydock/exception/DrydockResourceLockException.php
@@ -0,0 +1,4 @@
+<?php
+
+final class DrydockResourceLockException
+ extends Exception {}
diff --git a/src/applications/drydock/storage/DrydockLease.php b/src/applications/drydock/storage/DrydockLease.php
--- a/src/applications/drydock/storage/DrydockLease.php
+++ b/src/applications/drydock/storage/DrydockLease.php
@@ -213,30 +213,75 @@
}
}
- $this->openTransaction();
+ // Before we associate the lease with the resource, we lock the resource
+ // and reload it to make sure it is still pending or active. If we don't
+ // do this, the resource may have just been reclaimed. (Once we acquire
+ // the resource that stops it from being released, so we're nearly safe.)
+
+ $resource_phid = $resource->getPHID();
+ $hash = PhabricatorHash::digestForIndex($resource_phid);
+ $lock_key = 'drydock.resource:'.$hash;
+ $lock = PhabricatorGlobalLock::newLock($lock_key);
try {
- DrydockSlotLock::acquireLocks($this->getPHID(), $this->slotLocks);
- $this->slotLocks = array();
- } catch (DrydockSlotLockException $ex) {
- $this->killTransaction();
+ $lock->lock(15);
+ } catch (Exception $ex) {
+ throw new DrydockResourceLockException(
+ pht(
+ 'Failed to acquire lock for resource ("%s") while trying to '.
+ 'acquire lease ("%s").',
+ $resource->getPHID(),
+ $this->getPHID()));
+ }
- $this->logEvent(
- DrydockSlotLockFailureLogType::LOGCONST,
- array(
- 'locks' => $ex->getLockMap(),
- ));
+ $resource->reload();
- throw $ex;
+ if (($resource->getStatus() !== DrydockResourceStatus::STATUS_ACTIVE) &&
+ ($resource->getStatus() !== DrydockResourceStatus::STATUS_PENDING)) {
+ throw new DrydockAcquiredBrokenResourceException(
+ pht(
+ 'Trying to acquire lease ("%s") on a resource ("%s") in the '.
+ 'wrong status ("%s").',
+ $this->getPHID(),
+ $resource->getPHID(),
+ $resource->getStatus()));
}
- $this
- ->setResourcePHID($resource->getPHID())
- ->attachResource($resource)
- ->setStatus($new_status)
- ->save();
+ $caught = null;
+ try {
+ $this->openTransaction();
- $this->saveTransaction();
+ try {
+ DrydockSlotLock::acquireLocks($this->getPHID(), $this->slotLocks);
+ $this->slotLocks = array();
+ } catch (DrydockSlotLockException $ex) {
+ $this->killTransaction();
+
+ $this->logEvent(
+ DrydockSlotLockFailureLogType::LOGCONST,
+ array(
+ 'locks' => $ex->getLockMap(),
+ ));
+
+ throw $ex;
+ }
+
+ $this
+ ->setResourcePHID($resource->getPHID())
+ ->attachResource($resource)
+ ->setStatus($new_status)
+ ->save();
+
+ $this->saveTransaction();
+ } catch (Exception $ex) {
+ $caught = $ex;
+ }
+
+ $lock->unlock();
+
+ if ($caught) {
+ throw $caught;
+ }
$this->isAcquired = true;
diff --git a/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php b/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php
--- a/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php
+++ b/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php
@@ -53,6 +53,7 @@
$lease
->setStatus(DrydockLeaseStatus::STATUS_PENDING)
+ ->setResourcePHID(null)
->save();
$lease->logEvent(
@@ -301,23 +302,38 @@
$resources = $this->rankResources($resources, $lease);
$exceptions = array();
+ $yields = array();
$allocated = false;
foreach ($resources as $resource) {
try {
$this->acquireLease($resource, $lease);
$allocated = true;
break;
+ } catch (DrydockResourceLockException $ex) {
+ // We need to lock the resource to actually acquire it. If we aren't
+ // able to acquire the lock quickly enough, we can yield and try again
+ // later.
+ $yields[] = $ex;
+ } catch (DrydockAcquiredBrokenResourceException $ex) {
+ // If a resource was reclaimed or destroyed by the time we actually
+ // got around to acquiring it, we just got unlucky. We can yield and
+ // try again later.
+ $yields[] = $ex;
} catch (Exception $ex) {
$exceptions[] = $ex;
}
}
if (!$allocated) {
- throw new PhutilAggregateException(
- pht(
- 'Unable to acquire lease "%s" on any resource.',
- $lease->getPHID()),
- $exceptions);
+ if ($yields) {
+ throw new PhabricatorWorkerYieldException(15);
+ } else {
+ throw new PhutilAggregateException(
+ pht(
+ 'Unable to acquire lease "%s" on any resource.',
+ $lease->getPHID()),
+ $exceptions);
+ }
}
}

File Metadata

Mime Type
text/plain
Expires
Thu, Mar 6, 3:24 AM (2 w, 4 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7228677
Default Alt Text
D19080.diff (6 KB)

Event Timeline

Phabricator · Built by Phacility