From 4d55067fd87f57f724c29a4f685e2de80ce4fbf6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 26 Feb 2020 11:48:17 -0800 Subject: [PATCH] Make the "result" property on Future private Summary: Depends on D21033. Ref T11968. This is just an incremental step in modernizing Future and making it more robust. Currently, subclasses are expected to write directly to `$this->result`, but this isn't consistent with how modern classes generally work. Test Plan: Ran unit tests, created this revision. Maniphest Tasks: T11968 Differential Revision: https://secure.phabricator.com/D21034 --- src/future/Future.php | 54 ++++++++++++++++++-------- src/future/FutureProxy.php | 13 ++----- src/future/ImmediateFuture.php | 2 +- src/future/exec/ExecFuture.php | 23 ++++++++--- src/future/exec/PhutilExecPassthru.php | 11 ++---- src/future/http/HTTPFuture.php | 22 ++++++----- src/future/http/HTTPSFuture.php | 10 ++--- 7 files changed, 80 insertions(+), 55 deletions(-) diff --git a/src/future/Future.php b/src/future/Future.php index e468d076..08772279 100644 --- a/src/future/Future.php +++ b/src/future/Future.php @@ -7,7 +7,9 @@ */ abstract class Future extends Phobject { - protected $result; + private $hasResult = false; + private $result; + protected $exception; /** @@ -79,22 +81,6 @@ abstract class Future extends Phobject { } - /** - * Retrieve the final result of the future. This method will be called after - * the future is ready (as per @{method:isReady}) but before results are - * passed back to the caller. The major use of this function is that you can - * override it in subclasses to do postprocessing or error checking, which is - * particularly useful if building application-specific futures on top of - * primitive transport futures (like @{class:CurlFuture} and - * @{class:ExecFuture}) which can make it tricky to hook this logic into the - * main pipeline. - * - * @return mixed Final resolution of this future. - */ - protected function getResult() { - return $this->result; - } - /** * Default amount of time to wait on stream select for this future. Normally * 1 second is fine, but if the future has a timeout sooner than that it @@ -109,4 +95,38 @@ abstract class Future extends Phobject { return $this; } + /** + * Retrieve the final result of the future. + * + * @return wild Final resolution of this future. + */ + final protected function getResult() { + if (!$this->hasResult()) { + throw new Exception( + pht( + 'Future has not yet resolved. Resolve futures before retrieving '. + 'results.')); + } + + return $this->result; + } + + final protected function setResult($result) { + if ($this->hasResult()) { + throw new Exception( + pht( + 'Future has already resolved. Futures may not resolve more than '. + 'once.')); + } + + $this->hasResult = true; + $this->result = $result; + + return $this; + } + + final protected function hasResult() { + return $this->hasResult; + } + } diff --git a/src/future/FutureProxy.php b/src/future/FutureProxy.php index 37b39084..4b90df40 100644 --- a/src/future/FutureProxy.php +++ b/src/future/FutureProxy.php @@ -31,7 +31,9 @@ abstract class FutureProxy extends Future { } public function resolve() { - $this->getProxiedFuture()->resolve(); + $result = $this->getProxiedFuture()->resolve(); + $result = $this->didReceiveResult($result); + $this->setResult($result); return $this->getResult(); } @@ -52,15 +54,6 @@ abstract class FutureProxy extends Future { return $this->getProxiedFuture()->getWriteSockets(); } - protected function getResult() { - if ($this->result === null) { - $result = $this->getProxiedFuture()->resolve(); - $result = $this->didReceiveResult($result); - $this->result = $result; - } - return $this->result; - } - public function start() { $this->getProxiedFuture()->start(); return $this; diff --git a/src/future/ImmediateFuture.php b/src/future/ImmediateFuture.php index a5fe0a33..b2dbc4a9 100644 --- a/src/future/ImmediateFuture.php +++ b/src/future/ImmediateFuture.php @@ -7,7 +7,7 @@ final class ImmediateFuture extends Future { public function __construct($result) { - $this->result = $result; + $this->setResult($result); } public function isReady() { diff --git a/src/future/exec/ExecFuture.php b/src/future/exec/ExecFuture.php index 54b8d60f..df8ce772 100644 --- a/src/future/exec/ExecFuture.php +++ b/src/future/exec/ExecFuture.php @@ -381,22 +381,25 @@ final class ExecFuture extends PhutilExecutableFuture { * @task resolve */ public function resolveKill() { - if (!$this->result) { + if (!$this->hasResult()) { $signal = 9; if ($this->proc) { proc_terminate($this->proc, $signal); } - $this->result = array( + $result = array( 128 + $signal, $this->stdout, $this->stderr, ); + + $this->setResult($result); + $this->closeProcess(); } - return $this->result; + return $this->getResult(); } @@ -764,11 +767,14 @@ final class ExecFuture extends PhutilExecutableFuture { } } - $this->result = array( + $result = array( $err, $this->stdout, $this->stderr, ); + + $this->setResult($result); + $this->closeProcess(); return true; } @@ -839,11 +845,18 @@ final class ExecFuture extends PhutilExecutableFuture { unset($this->windowsStderrTempFile); if ($this->profilerCallID !== null) { + if ($this->hasResult()) { + $result = $this->getResult(); + $err = idx($result, 0); + } else { + $err = null; + } + $profiler = PhutilServiceProfiler::getInstance(); $profiler->endServiceCall( $this->profilerCallID, array( - 'err' => $this->result ? idx($this->result, 0) : null, + 'err' => $err, )); $this->profilerCallID = null; } diff --git a/src/future/exec/PhutilExecPassthru.php b/src/future/exec/PhutilExecPassthru.php index 721817bc..8a761923 100644 --- a/src/future/exec/PhutilExecPassthru.php +++ b/src/future/exec/PhutilExecPassthru.php @@ -20,8 +20,6 @@ */ final class PhutilExecPassthru extends PhutilExecutableFuture { - private $passthruResult; - /* -( Executing Passthru Commands )---------------------------------------- */ @@ -105,15 +103,12 @@ final class PhutilExecPassthru extends PhutilExecutableFuture { // full control of the console. We're just implementing the interfaces to // make it easier to share code with ExecFuture. - if ($this->passthruResult === null) { - $this->passthruResult = $this->execute(); + if (!$this->hasResult()) { + $result = $this->execute(); + $this->setResult($result); } return true; } - protected function getResult() { - return $this->passthruResult; - } - } diff --git a/src/future/http/HTTPFuture.php b/src/future/http/HTTPFuture.php index bfe5466d..738cde27 100644 --- a/src/future/http/HTTPFuture.php +++ b/src/future/http/HTTPFuture.php @@ -180,8 +180,9 @@ final class HTTPFuture extends BaseHTTPFuture { if (!$socket) { $this->stateReady = true; - $this->result = $this->buildErrorResult( - HTTPFutureTransportResponseStatus::ERROR_CONNECTION_FAILED); + $this->setResult( + $this->buildErrorResult( + HTTPFutureTransportResponseStatus::ERROR_CONNECTION_FAILED)); return null; } @@ -209,16 +210,19 @@ final class HTTPFuture extends BaseHTTPFuture { $this->stateReady = true; if ($timeout) { - $this->result = $this->buildErrorResult( - HTTPFutureTransportResponseStatus::ERROR_TIMEOUT); + $this->setResult( + $this->buildErrorResult( + HTTPFutureTransportResponseStatus::ERROR_TIMEOUT)); } else if (!$this->stateConnected) { - $this->result = $this->buildErrorResult( - HTTPFutureTransportResponseStatus::ERROR_CONNECTION_REFUSED); + $this->setResult( + $this->buildErrorResult( + HTTPFutureTransportResponseStatus::ERROR_CONNECTION_REFUSED)); } else if (!$this->stateWriteComplete) { - $this->result = $this->buildErrorResult( - HTTPFutureTransportResponseStatus::ERROR_CONNECTION_FAILED); + $this->setResult( + $this->buildErrorResult( + HTTPFutureTransportResponseStatus::ERROR_CONNECTION_FAILED)); } else { - $this->result = $this->parseRawHTTPResponse($this->response); + $this->setResult($this->parseRawHTTPResponse($this->response)); } $profiler = PhutilServiceProfiler::getInstance(); diff --git a/src/future/http/HTTPSFuture.php b/src/future/http/HTTPSFuture.php index 7dca7eda..f65af3e8 100644 --- a/src/future/http/HTTPSFuture.php +++ b/src/future/http/HTTPSFuture.php @@ -194,7 +194,7 @@ final class HTTPSFuture extends BaseHTTPFuture { } public function isReady() { - if (isset($this->result)) { + if ($this->hasResult()) { return true; } @@ -476,7 +476,7 @@ final class HTTPSFuture extends BaseHTTPFuture { $body = null; $headers = array(); - $this->result = array($status, $body, $headers); + $this->setResult(array($status, $body, $headers)); } else if ($this->parser) { $streaming_parser = $this->parser; try { @@ -491,12 +491,12 @@ final class HTTPSFuture extends BaseHTTPFuture { $result = array($ex, null, array()); } - $this->result = $result; + $this->setResult($result); } else { // cURL returns headers of all redirects, we strip all but the final one. $redirects = curl_getinfo($curl, CURLINFO_REDIRECT_COUNT); $result = preg_replace('/^(.*\r\n\r\n){'.$redirects.'}/sU', '', $result); - $this->result = $this->parseRawHTTPResponse($result); + $this->setResult($this->parseRawHTTPResponse($result)); } curl_multi_remove_handle(self::$multi, $curl); @@ -518,7 +518,7 @@ final class HTTPSFuture extends BaseHTTPFuture { $sink = $this->getProgressSink(); if ($sink) { - $status = head($this->result); + $status = head($this->getResult()); if ($status->isError()) { $sink->didFailWork(); } else {