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

@@ -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();
+  }
+
 }

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();
   }
 
   /**

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;

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");