From a745055813e4c4a3687df82ebbd4fb5cfc54a89b Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 12 Aug 2020 11:11:07 -0700 Subject: [PATCH] Lift Diffusion Conduit call proxying to the root level of Conduit Summary: Ref T13552. Some Diffusion conduit calls may only be served by a node which hosts a working copy on disk, so they're proxied if received by a different node. This capability is currently bound tightly to "DiffusionRequest", which is a bundle of context parameters used by some Diffusion calls. However, call proxying is not fundamentally a Diffusion behavior. I want to perform proxying on a "*.search" call which does not use the "DiffusionRequest" parameter bundle. Lift proxying to the root level of Conduit. Test Plan: Browsed diffusion in a clusterized repsository. Maniphest Tasks: T13552 Differential Revision: https://secure.phabricator.com/D21442 --- .../conduit/method/ConduitAPIMethod.php | 12 +++ .../conduit/protocol/ConduitAPIRequest.php | 4 + .../DiffusionQueryConduitAPIMethod.php | 75 +++++++++---------- .../storage/PhabricatorRepository.php | 17 +++++ 4 files changed, 70 insertions(+), 38 deletions(-) diff --git a/src/applications/conduit/method/ConduitAPIMethod.php b/src/applications/conduit/method/ConduitAPIMethod.php index 0fbfaa2fc3..090235500d 100644 --- a/src/applications/conduit/method/ConduitAPIMethod.php +++ b/src/applications/conduit/method/ConduitAPIMethod.php @@ -120,9 +120,21 @@ abstract class ConduitAPIMethod public function executeMethod(ConduitAPIRequest $request) { $this->setViewer($request->getUser()); + $client = $this->newConduitCallProxyClient($request); + if ($client) { + // We're proxying, so just make an intracluster call. + return $client->callMethodSynchronous( + $this->getAPIMethodName(), + $request->getAllParameters()); + } + return $this->execute($request); } + protected function newConduitCallProxyClient(ConduitAPIRequest $request) { + return null; + } + abstract public function getAPIMethodName(); /** diff --git a/src/applications/conduit/protocol/ConduitAPIRequest.php b/src/applications/conduit/protocol/ConduitAPIRequest.php index 3a2818a47a..78988f2b3b 100644 --- a/src/applications/conduit/protocol/ConduitAPIRequest.php +++ b/src/applications/conduit/protocol/ConduitAPIRequest.php @@ -51,6 +51,10 @@ final class ConduitAPIRequest extends Phobject { return $this->user; } + public function getViewer() { + return $this->getUser(); + } + public function setOAuthToken( PhabricatorOAuthServerAccessToken $oauth_token) { $this->oauthToken = $oauth_token; diff --git a/src/applications/diffusion/conduit/DiffusionQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionQueryConduitAPIMethod.php index ca32cc0127..cd6a45360b 100644 --- a/src/applications/diffusion/conduit/DiffusionQueryConduitAPIMethod.php +++ b/src/applications/diffusion/conduit/DiffusionQueryConduitAPIMethod.php @@ -85,6 +85,35 @@ abstract class DiffusionQueryConduitAPIMethod * should occur after @{method:getResult}, like formatting a timestamp. */ final protected function execute(ConduitAPIRequest $request) { + $drequest = $this->getDiffusionRequest(); + + // We pass this flag on to prevent proxying of any other Conduit calls + // which we need to make in order to respond to this one. Although we + // could safely proxy them, we take a big performance hit in the common + // case, and doing more proxying wouldn't exercise any additional code so + // we wouldn't gain a testability/predictability benefit. + $is_cluster_request = $request->getIsClusterRequest(); + $drequest->setIsClusterRequest($is_cluster_request); + + $viewer = $request->getViewer(); + $repository = $drequest->getRepository(); + + // TODO: Allow web UI queries opt out of this if they don't care about + // fetching the most up-to-date data? Synchronization can be slow, and a + // lot of web reads are probably fine if they're a few seconds out of + // date. + id(new DiffusionRepositoryClusterEngine()) + ->setViewer($viewer) + ->setRepository($repository) + ->synchronizeWorkingCopyBeforeRead(); + + return $this->getResult($request); + } + + + protected function newConduitCallProxyClient(ConduitAPIRequest $request) { + $viewer = $request->getViewer(); + $identifier = $request->getValue('repository'); if ($identifier === null) { $identifier = $request->getValue('callsign'); @@ -92,7 +121,7 @@ abstract class DiffusionQueryConduitAPIMethod $drequest = DiffusionRequest::newFromDictionary( array( - 'user' => $request->getUser(), + 'user' => $viewer, 'repository' => $identifier, 'branch' => $request->getValue('branch'), 'path' => $request->getValue('path'), @@ -106,46 +135,16 @@ abstract class DiffusionQueryConduitAPIMethod $identifier)); } - // Figure out whether we're going to handle this request on this device, - // or proxy it to another node in the cluster. - - // If this is a cluster request and we need to proxy, we'll explode here - // to prevent infinite recursion. - - $is_cluster_request = $request->getIsClusterRequest(); - $viewer = $request->getUser(); - $repository = $drequest->getRepository(); - $client = $repository->newConduitClient( - $viewer, - $is_cluster_request); + + $client = $repository->newConduitClientForRequest($request); if ($client) { - // We're proxying, so just make an intracluster call. - return $client->callMethodSynchronous( - $this->getAPIMethodName(), - $request->getAllParameters()); - } else { - - // We pass this flag on to prevent proxying of any other Conduit calls - // which we need to make in order to respond to this one. Although we - // could safely proxy them, we take a big performance hit in the common - // case, and doing more proxying wouldn't exercise any additional code so - // we wouldn't gain a testability/predictability benefit. - $drequest->setIsClusterRequest($is_cluster_request); - - $this->setDiffusionRequest($drequest); - - // TODO: Allow web UI queries opt out of this if they don't care about - // fetching the most up-to-date data? Synchronization can be slow, and a - // lot of web reads are probably fine if they're a few seconds out of - // date. - id(new DiffusionRepositoryClusterEngine()) - ->setViewer($viewer) - ->setRepository($repository) - ->synchronizeWorkingCopyBeforeRead(); - - return $this->getResult($request); + return $client; } + + $this->setDiffusionRequest($drequest); + + return null; } protected function getResult(ConduitAPIRequest $request) { diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index 05009c0d91..71afc56281 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -2241,6 +2241,23 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO return $client; } + public function newConduitClientForRequest(ConduitAPIRequest $request) { + // Figure out whether we're going to handle this request on this device, + // or proxy it to another node in the cluster. + + // If this is a cluster request and we need to proxy, we'll explode here + // to prevent infinite recursion. + + $viewer = $request->getViewer(); + $is_cluster_request = $request->getIsClusterRequest(); + + $client = $this->newConduitClient( + $viewer, + $is_cluster_request); + + return $client; + } + public function getPassthroughEnvironmentalVariables() { $env = $_ENV;