Page MenuHomePhabricator
Files F14029467

D14818.diff
No OneTemporary
Actions

D14818.diff
View Options

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
@@ -26,6 +26,7 @@
$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
--- a/src/applications/drydock/worker/DrydockResourceUpdateWorker.php
+++ b/src/applications/drydock/worker/DrydockResourceUpdateWorker.php
@@ -24,6 +24,7 @@
$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
--- a/src/applications/drydock/worker/DrydockWorker.php
+++ b/src/applications/drydock/worker/DrydockWorker.php
@@ -170,4 +170,30 @@
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
--- a/src/infrastructure/daemon/workers/PhabricatorWorker.php
+++ b/src/infrastructure/daemon/workers/PhabricatorWorker.php
@@ -158,11 +158,8 @@
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,12 +233,35 @@
*
* @return list<tuple<string, wild, int|null>> 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<string, wild> 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.
*
* Reschedules the specified tasks if they are currently queued in a yielded,
diff --git a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php
--- a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php
+++ b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php
@@ -216,16 +216,11 @@
// 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;

File Metadata

Mime Type
text/plain
Expires
Sat, Nov 9, 8:44 PM (1 w, 2 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6714879
Default Alt Text
D14818.diff (4 KB)

Event Timeline

Phabricator · Built by Phacility