From 78d1b62bb8e9b5d90b005626a1635718c5273ce3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 23 Jul 2020 09:11:30 -0700 Subject: [PATCH] Streamline handling of Futures and PIDs in daemons Summary: Ref T13555. Currently, the daemon future may resolve into a failure state immediately inside "start()", and not have a valid PID when we read it. Instead, read PIDs from the current active future in all cases, using "hasPID()" to test for the presence of a valid PID. Since we don't query the PID immediately, we no longer need to explicitly start the future. Also fix an issue where the same future could be added to the overseer pool more than once if it threw on "resolve()". In general: - Before we "resolve()" a future, detach it from the DaemonHandle: we're always done with it. - Catch exceptions on resolution and treat them the same way as subprocess resolution errors. These aren't common, but are possible in the general case. - Have DaemonHandle add futures to the future pool directly when they're created. Test Plan: - Ran daemons with intentional subprocess creation failures, saw clean recovery. - Ran daemons with intentional resolution exceptions, saw clean recovery. Maniphest Tasks: T13555 Differential Revision: https://secure.phabricator.com/D21425 --- .../daemon/PhutilDaemonHandle.php | 67 +++++++++++++------ .../daemon/PhutilDaemonOverseer.php | 9 +-- .../daemon/PhutilDaemonPool.php | 12 ---- 3 files changed, 51 insertions(+), 37 deletions(-) diff --git a/src/infrastructure/daemon/PhutilDaemonHandle.php b/src/infrastructure/daemon/PhutilDaemonHandle.php index 25c517b8cf..428a64a056 100644 --- a/src/infrastructure/daemon/PhutilDaemonHandle.php +++ b/src/infrastructure/daemon/PhutilDaemonHandle.php @@ -16,7 +16,6 @@ final class PhutilDaemonHandle extends Phobject { private $restartAt; private $busyEpoch; - private $pid; private $daemonID; private $deadline; private $heartbeat; @@ -104,7 +103,7 @@ final class PhutilDaemonHandle extends Phobject { } public function isRunning() { - return (bool)$this->future; + return (bool)$this->getFuture(); } public function isHibernating() { @@ -134,10 +133,6 @@ final class PhutilDaemonHandle extends Phobject { return (!$this->shouldRestart && !$this->isRunning()); } - public function getFuture() { - return $this->future; - } - public function update() { if (!$this->isRunning()) { if (!$this->shouldRestart) { @@ -152,11 +147,19 @@ final class PhutilDaemonHandle extends Phobject { $this->startDaemonProcess(); } - $future = $this->future; + $future = $this->getFuture(); $result = null; - if ($future->isReady()) { - $result = $future->resolve(); + $caught = null; + if ($future->canResolve()) { + $this->future = null; + try { + $result = $future->resolve(); + } catch (Exception $ex) { + $caught = $ex; + } catch (Throwable $ex) { + $caught = $ex; + } } list($stdout, $stderr) = $future->read(); @@ -173,16 +176,22 @@ final class PhutilDaemonHandle extends Phobject { } } - if ($result !== null) { - list($err) = $result; + if ($result !== null || $caught !== null) { - if ($err) { - $this->logMessage('FAIL', pht('Process exited with error %s.', $err)); + if ($caught) { + $message = pht( + 'Process failed with exception: %s', + $caught->getMessage()); + $this->logMessage('FAIL', $message); } else { - $this->logMessage('DONE', pht('Process exited normally.')); - } + list($err) = $result; - $this->future = null; + if ($err) { + $this->logMessage('FAIL', pht('Process exited with error %s.', $err)); + } else { + $this->logMessage('DONE', pht('Process exited normally.')); + } + } if ($this->shouldShutdown) { $this->restartAt = null; @@ -244,8 +253,22 @@ final class PhutilDaemonHandle extends Phobject { return $this->daemonID; } - public function getPID() { - return $this->pid; + private function getFuture() { + return $this->future; + } + + private function getPID() { + $future = $this->getFuture(); + + if (!$future) { + return null; + } + + if (!$future->hasPID()) { + return null; + } + + return $future->getPID(); } private function getCaptureBufferSize() { @@ -346,10 +369,12 @@ final class PhutilDaemonHandle extends Phobject { $this->stdoutBuffer = ''; $this->hibernating = false; - $this->future = $this->newExecFuture(); - $this->future->start(); + $future = $this->newExecFuture(); + $this->future = $future; - $this->pid = $this->future->getPID(); + $pool = $this->getDaemonPool(); + $overseer = $pool->getOverseer(); + $overseer->addFutureToPool($future); } private function didReadStdout($data) { diff --git a/src/infrastructure/daemon/PhutilDaemonOverseer.php b/src/infrastructure/daemon/PhutilDaemonOverseer.php index 77f6f6a20a..974a9cf3ed 100644 --- a/src/infrastructure/daemon/PhutilDaemonOverseer.php +++ b/src/infrastructure/daemon/PhutilDaemonOverseer.php @@ -181,10 +181,6 @@ EOHELP } } - foreach ($pool->getFutures() as $future) { - $future_pool->addFuture($future); - } - if ($pool->getDaemons()) { $running_pools = true; } @@ -210,6 +206,11 @@ EOHELP exit($this->err); } + public function addFutureToPool(Future $future) { + $this->getFuturePool()->addFuture($future); + return $this; + } + private function getFuturePool() { if (!$this->futurePool) { $pool = new FuturePool(); diff --git a/src/infrastructure/daemon/PhutilDaemonPool.php b/src/infrastructure/daemon/PhutilDaemonPool.php index 50b22289a8..9cdc15228e 100644 --- a/src/infrastructure/daemon/PhutilDaemonPool.php +++ b/src/infrastructure/daemon/PhutilDaemonPool.php @@ -111,18 +111,6 @@ final class PhutilDaemonPool extends Phobject { return $this->daemons; } - public function getFutures() { - $futures = array(); - foreach ($this->getDaemons() as $daemon) { - $future = $daemon->getFuture(); - if ($future) { - $futures[] = $future; - } - } - - return $futures; - } - public function didReceiveSignal($signal, $signo) { switch ($signal) { case PhutilDaemonOverseer::SIGNAL_GRACEFUL: -- 2.51.2