From ceb082ef6b2919d76a90d4a53ca84f5b1e0c2c06 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 23 Jul 2020 08:26:32 -0700 Subject: [PATCH] Give Futures clearer start/end and exception semantics Summary: Ref T13555. Currently: - If an exception is raised in "start()", the exception state is not set on the future. - Futures do not always call "startFuture()" before starting, and do not always call "endFuture()" once they become resolvable. - If you start an ExecFuture which immediately fails and then call "getPID()" on it, you get an unclear exception. Simplify these behaviors: - In FutureIterator, only start futures which have not already started. - When starting a future on any pathway, run start code. - When a future becomes resolvable on any pathway, run end code. - Raise a more clear exception when calling "getPID()" on a future with no subprocess. Test Plan: Faked a failing subprocess with "$proc = null", ran "bin/phd debug taskmaster" etc. Got clearer errors and more consistent future lifecycle workflows. Maniphest Tasks: T13555 Differential Revision: https://secure.phabricator.com/D21423 --- src/future/Future.php | 102 +++++++++++------- src/future/FutureIterator.php | 17 ++- src/future/exec/ExecFuture.php | 25 ++++- .../query/ArcanistGitCommitGraphQuery.php | 2 +- 4 files changed, 94 insertions(+), 52 deletions(-) diff --git a/src/future/Future.php b/src/future/Future.php index 8078aacc..b8656bd6 100644 --- a/src/future/Future.php +++ b/src/future/Future.php @@ -14,6 +14,7 @@ abstract class Future extends Phobject { private $exception; private $futureKey; private $serviceProfilerCallID; + private $raiseExceptionOnStart = true; private static $nextKey = 1; /** @@ -41,7 +42,7 @@ abstract class Future extends Phobject { 'timeout.')); } - if (!$this->hasResult() && !$this->hasException()) { + if (!$this->canResolve()) { $graph = new FutureIterator(array($this)); $graph->resolveAll(); } @@ -53,25 +54,8 @@ abstract class Future extends Phobject { return $this->getResult(); } - final public function startFuture() { - if ($this->hasStarted) { - throw new Exception( - pht( - 'Future has already started; futures can not start more '. - 'than once.')); - } - $this->hasStarted = true; - - $this->startServiceProfiler(); - $this->updateFuture(); - } - final public function updateFuture() { - if ($this->hasException()) { - return; - } - - if ($this->hasResult()) { + if ($this->canResolve()) { return; } @@ -84,25 +68,6 @@ abstract class Future extends Phobject { } } - final public function endFuture() { - if (!$this->hasException() && !$this->hasResult()) { - throw new Exception( - pht( - 'Trying to end a future which has no exception and no result. '. - 'Futures must resolve before they can be ended.')); - } - - if ($this->hasEnded) { - throw new Exception( - pht( - 'Future has already ended; futures can not end more '. - 'than once.')); - } - $this->hasEnded = true; - - $this->endServiceProfiler(); - } - private function startServiceProfiler() { // NOTE: This is a soft dependency so that we don't need to build the @@ -181,7 +146,24 @@ abstract class Future extends Phobject { } public function start() { - $this->isReady(); + if ($this->hasStarted) { + throw new Exception( + pht( + 'Future has already started; futures can not start more '. + 'than once.')); + } + $this->hasStarted = true; + + $this->startServiceProfiler(); + + $this->updateFuture(); + + if ($this->raiseExceptionOnStart) { + if ($this->hasException()) { + throw $this->getException(); + } + } + return $this; } @@ -212,6 +194,8 @@ abstract class Future extends Phobject { $this->hasResult = true; $this->result = $result; + $this->endFuture(); + return $this; } @@ -219,13 +203,16 @@ abstract class Future extends Phobject { return $this->hasResult; } - final private function setException($exception) { + private function setException($exception) { // NOTE: The parameter may be an Exception or a Throwable. $this->exception = $exception; + + $this->endFuture(); + return $this; } - final private function getException() { + private function getException() { return $this->exception; } @@ -254,4 +241,37 @@ abstract class Future extends Phobject { return $this->futureKey; } + final public function setRaiseExceptionOnStart($raise) { + $this->raiseExceptionOnStart = $raise; + return $this; + } + + final public function getHasFutureStarted() { + return $this->hasStarted; + } + + final public function canResolve() { + if ($this->hasResult()) { + return true; + } + + if ($this->hasException()) { + return true; + } + + return false; + } + + private function endFuture() { + if ($this->hasEnded) { + throw new Exception( + pht( + 'Future has already ended; futures can not end more '. + 'than once.')); + } + $this->hasEnded = true; + + $this->endServiceProfiler(); + } + } diff --git a/src/future/FutureIterator.php b/src/future/FutureIterator.php index e33282c8..028c02da 100644 --- a/src/future/FutureIterator.php +++ b/src/future/FutureIterator.php @@ -222,12 +222,7 @@ final class FutureIterator $resolve_key = null; foreach ($working_set as $future_key => $future) { - if ($future->hasException()) { - $resolve_key = $future_key; - break; - } - - if ($future->hasResult()) { + if ($future->canResolve()) { $resolve_key = $future_key; break; } @@ -393,7 +388,13 @@ final class FutureIterator unset($this->wait[$future_key]); $this->work[$future_key] = $future_key; - $this->futures[$future_key]->startFuture(); + $future = $this->futures[$future_key]; + + if (!$future->getHasFutureStarted()) { + $future + ->setRaiseExceptionOnStart(false) + ->start(); + } } private function moveFutureToDone($future_key) { @@ -404,8 +405,6 @@ final class FutureIterator // futures that are ready to go as soon as we can. $this->updateWorkingSet(); - - $this->futures[$future_key]->endFuture(); } /** diff --git a/src/future/exec/ExecFuture.php b/src/future/exec/ExecFuture.php index 0264d4d3..ecce090c 100644 --- a/src/future/exec/ExecFuture.php +++ b/src/future/exec/ExecFuture.php @@ -95,6 +95,18 @@ final class ExecFuture extends PhutilExecutableFuture { return $status['pid']; } + public function hasPID() { + if ($this->procStatus) { + return true; + } + + if ($this->proc) { + return true; + } + + return false; + } + /* -( Configuring Execution )---------------------------------------------- */ @@ -194,7 +206,7 @@ final class ExecFuture extends PhutilExecutableFuture { public function readStdout() { if ($this->start) { - $this->isReady(); // Sync + $this->updateFuture(); // Sync } $result = (string)substr($this->stdout, $this->stdoutPos); @@ -890,6 +902,17 @@ final class ExecFuture extends PhutilExecutableFuture { return $this->procStatus; } } + + // See T13555. This may occur if you call "getPID()" on a future which + // exited immediately without ever creating a valid subprocess. + + if (!$this->proc) { + throw new Exception( + pht( + 'Attempting to get subprocess status in "ExecFuture" with no '. + 'valid subprocess.')); + } + $this->procStatus = proc_get_status($this->proc); return $this->procStatus; diff --git a/src/repository/graph/query/ArcanistGitCommitGraphQuery.php b/src/repository/graph/query/ArcanistGitCommitGraphQuery.php index 2491a549..53587e45 100644 --- a/src/repository/graph/query/ArcanistGitCommitGraphQuery.php +++ b/src/repository/graph/query/ArcanistGitCommitGraphQuery.php @@ -104,7 +104,7 @@ final class ArcanistGitCommitGraphQuery } $future = array_pop($this->futures); - $future->startFuture(); + $future->start(); $iterator = id(new LinesOfALargeExecFuture($future)) ->setDelimiter("\1");