From e9af4f89707922b24282a8b4e94d3ce759b86731 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 18 Dec 2015 06:38:02 -0800 Subject: [PATCH] Fix an issue where Drydock followup tasks would not queue if the main task failed Summary: Ref T9994. This fixes the first issue discussed on that task, which is that when a merge fails after "arc land", we would not clean up all the leases properly. Specifically, when a merge fails, we use `queueTask()` to schedule a followup task. This followup destroys the lease and frees the underlying resource. However, the default behavior of `queueTask()` is to //not queue tasks// if the parent task fails. This is a reasonable, safe behavior that was originally introduced in D8774, where it kept us from sending too much mail if a task did "send some mail" and then failed a little later on and got retried. Since I think the default behavior is correct, I just special cased the behavior for Drydock to make it queue even on failure. These are the only types of followup tasks we currently want to queue on main task failure. (It's possible that future Blueprints might want some kind of more specialized behavior, where some tasks queue only on success, but we can cross that bridge when we come to it.) Test Plan: - See T9994#149878 for test case setup. - I ran that test case again with this patch, and saw the followup task queue properly in the `--trace` log, a correspoinding update task show up in `/daemon/`, and the lease get destroyed when I ran it a moment later. {F1029915} Reviewers: chad Reviewed By: chad Maniphest Tasks: T9994 Differential Revision: https://secure.phabricator.com/D14818 --- .../worker/DrydockLeaseUpdateWorker.php | 1 + .../worker/DrydockResourceUpdateWorker.php | 1 + .../drydock/worker/DrydockWorker.php | 26 +++++++++++++++ .../daemon/workers/PhabricatorWorker.php | 32 +++++++++++++++---- .../storage/PhabricatorWorkerActiveTask.php | 15 +++------ 5 files changed, 59 insertions(+), 16 deletions(-) diff --git a/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php b/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php index 8f34a88342..80b2a6f30c 100644 --- a/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php +++ b/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php @@ -26,6 +26,7 @@ final class DrydockLeaseUpdateWorker extends DrydockWorker { $this->handleUpdate($lease); } catch (Exception $ex) { $lock->unlock(); + $this->flushDrydockTaskQueue(); throw $ex; } diff --git a/src/applications/drydock/worker/DrydockResourceUpdateWorker.php b/src/applications/drydock/worker/DrydockResourceUpdateWorker.php index e0deabdb1b..5d27e79d5c 100644 --- a/src/applications/drydock/worker/DrydockResourceUpdateWorker.php +++ b/src/applications/drydock/worker/DrydockResourceUpdateWorker.php @@ -24,6 +24,7 @@ final class DrydockResourceUpdateWorker extends DrydockWorker { $this->handleUpdate($resource); } catch (Exception $ex) { $lock->unlock(); + $this->flushDrydockTaskQueue(); throw $ex; } diff --git a/src/applications/drydock/worker/DrydockWorker.php b/src/applications/drydock/worker/DrydockWorker.php index 029cbf1c84..08c32be869 100644 --- a/src/applications/drydock/worker/DrydockWorker.php +++ b/src/applications/drydock/worker/DrydockWorker.php @@ -170,4 +170,30 @@ abstract class DrydockWorker extends PhabricatorWorker { return 15; } + protected function flushDrydockTaskQueue() { + // NOTE: By default, queued tasks are not scheduled if the current task + // fails. This is a good, safe default behavior. For example, it can + // protect us from executing side effect tasks too many times, like + // sending extra email. + + // However, it is not the behavior we want in Drydock, because we queue + // followup tasks after lease and resource failures and want them to + // execute in order to clean things up. + + // At least for now, we just explicitly flush the queue before exiting + // with a failure to make sure tasks get queued up properly. + try { + $this->flushTaskQueue(); + } catch (Exception $ex) { + // If this fails, we want to swallow the exception so the caller throws + // the original error, since we're more likely to be able to understand + // and fix the problem if we have the original error than if we replace + // it with this one. + phlog($ex); + } + + return $this; + } + + } diff --git a/src/infrastructure/daemon/workers/PhabricatorWorker.php b/src/infrastructure/daemon/workers/PhabricatorWorker.php index 473649ba19..ea59462740 100644 --- a/src/infrastructure/daemon/workers/PhabricatorWorker.php +++ b/src/infrastructure/daemon/workers/PhabricatorWorker.php @@ -158,11 +158,8 @@ abstract class PhabricatorWorker extends Phobject { while (true) { try { - $worker->doWork(); - foreach ($worker->getQueuedTasks() as $queued_task) { - list($queued_class, $queued_data, $queued_options) = $queued_task; - self::scheduleTask($queued_class, $queued_data, $queued_options); - } + $worker->executeTask(); + $worker->flushTaskQueue(); break; } catch (PhabricatorWorkerYieldException $ex) { phlog( @@ -236,11 +233,34 @@ abstract class PhabricatorWorker extends Phobject { * * @return list> Queued task specifications. */ - final public function getQueuedTasks() { + final protected function getQueuedTasks() { return $this->queuedTasks; } + /** + * Schedule any queued tasks, then empty the task queue. + * + * By default, the queue is flushed only if a task succeeds. You can call + * this method to force the queue to flush before failing (for example, if + * you are using queues to improve locking behavior). + * + * @param map Optional default options. + * @return this + */ + final public function flushTaskQueue($defaults = array()) { + foreach ($this->getQueuedTasks() as $task) { + list($class, $data, $options) = $task; + + $options = $options + $defaults; + + self::scheduleTask($class, $data, $options); + } + + $this->queuedTasks = array(); + } + + /** * Awaken tasks that have yielded. * diff --git a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php index 57a1794ec3..0fcc78db6e 100644 --- a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php +++ b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php @@ -216,16 +216,11 @@ final class PhabricatorWorkerActiveTask extends PhabricatorWorkerTask { // NOTE: If this throws, we don't want it to cause the task to fail again, // so execute it out here and just let the exception escape. if ($did_succeed) { - foreach ($worker->getQueuedTasks() as $task) { - list($class, $data, $options) = $task; - - // Default the new task priority to our own priority. - $options = $options + array( - 'priority' => (int)$this->getPriority(), - ); - - PhabricatorWorker::scheduleTask($class, $data, $options); - } + // Default the new task priority to our own priority. + $defaults = array( + 'priority' => (int)$this->getPriority(), + ); + $worker->flushTaskQueue($defaults); } return $result;