From e96cd29efff7ebfcbf765eb2c70a1a434c96bb91 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 18 Jun 2015 13:06:01 -0700 Subject: [PATCH] Reduce badness in viewing large files in the Diffusion web UI Summary: Fixes T8597. Second issue there is that if you look at a huge file in Diffusion (like `/path/to/300MB.pdf`) we pull the whole thing over Conduit upfront, then try to shove it into file storage. Instead, pull no more than the chunk limit (normally 4MB) and don't spend more than 10s pulling data. If we get 4MB of data and/or time out, just fail with a message in the vein of "this is a really big file". Eventually, we could improve this: - We can determine the //size// of very large files correctly in at least some VCSes, this just takes a little more work. This would let us show the true filesize, at least. - We could eventually stream the data out of the VCS, but we can't stream data over Conduit right now and this is a lot of work. This is just "stop crashing". Test Plan: Changed limits to 0.01 seconds and 8 bytes and saw reasonable errors. Changed them back and got normal beahvior. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T8597 Differential Revision: https://secure.phabricator.com/D13348 --- ...fusionFileContentQueryConduitAPIMethod.php | 16 +++++++ .../DiffusionBrowseFileController.php | 48 +++++++++++++++---- .../filecontent/DiffusionFileContentQuery.php | 46 +++++++++++++++++- 3 files changed, 101 insertions(+), 9 deletions(-) diff --git a/src/applications/diffusion/conduit/DiffusionFileContentQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionFileContentQueryConduitAPIMethod.php index 671ec3ef6f..61987b4d77 100644 --- a/src/applications/diffusion/conduit/DiffusionFileContentQueryConduitAPIMethod.php +++ b/src/applications/diffusion/conduit/DiffusionFileContentQueryConduitAPIMethod.php @@ -20,6 +20,8 @@ final class DiffusionFileContentQueryConduitAPIMethod 'path' => 'required string', 'commit' => 'required string', 'needsBlame' => 'optional bool', + 'timeout' => 'optional int', + 'byteLimit' => 'optional int', ); } @@ -31,16 +33,30 @@ final class DiffusionFileContentQueryConduitAPIMethod $file_query ->setViewer($request->getUser()) ->setNeedsBlame($needs_blame); + + $timeout = $request->getValue('timeout'); + if ($timeout) { + $file_query->setTimeout($timeout); + } + + $byte_limit = $request->getValue('byteLimit'); + if ($byte_limit) { + $file_query->setByteLimit($byte_limit); + } + $file_content = $file_query->loadFileContent(); + if ($needs_blame) { list($text_list, $rev_list, $blame_dict) = $file_query->getBlameData(); } else { $text_list = $rev_list = $blame_dict = array(); } + $file_content ->setBlameDict($blame_dict) ->setRevList($rev_list) ->setTextList($text_list); + return $file_content->toDictionary(); } diff --git a/src/applications/diffusion/controller/DiffusionBrowseFileController.php b/src/applications/diffusion/controller/DiffusionBrowseFileController.php index c55d217bfe..f975984789 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseFileController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseFileController.php @@ -54,14 +54,27 @@ final class DiffusionBrowseFileController extends DiffusionBrowseController { $needs_blame = ($show_blame && !$show_color) || ($show_blame && $request->isAjax()); + $params = array( + 'commit' => $drequest->getCommit(), + 'path' => $drequest->getPath(), + 'needsBlame' => $needs_blame, + ); + + $byte_limit = null; + if ($view !== 'raw') { + $byte_limit = PhabricatorFileStorageEngine::getChunkThreshold(); + $time_limit = 10; + + $params += array( + 'timeout' => $time_limit, + 'byteLimit' => $byte_limit, + ); + } + $file_content = DiffusionFileContent::newFromConduit( $this->callConduitWithDiffusionRequest( 'diffusion.filecontentquery', - array( - 'commit' => $drequest->getCommit(), - 'path' => $drequest->getPath(), - 'needsBlame' => $needs_blame, - ))); + $params)); $data = $file_content->getCorpus(); if ($view === 'raw') { @@ -71,8 +84,13 @@ final class DiffusionBrowseFileController extends DiffusionBrowseController { $this->loadLintMessages(); $this->coverage = $drequest->loadCoverage(); - $binary_uri = null; - if (ArcanistDiffUtils::isHeuristicBinaryFile($data)) { + if ($byte_limit && (strlen($data) == $byte_limit)) { + $corpus = $this->buildErrorCorpus( + pht( + 'This file is larger than %s byte(s), and too large to display '. + 'in the web UI.', + $byte_limit)); + } else if (ArcanistDiffUtils::isHeuristicBinaryFile($data)) { $file = $this->loadFileForData($path, $data); $file_uri = $file->getBestURI(); @@ -80,7 +98,6 @@ final class DiffusionBrowseFileController extends DiffusionBrowseController { $corpus = $this->buildImageCorpus($file_uri); } else { $corpus = $this->buildBinaryCorpus($file_uri, $data); - $binary_uri = $file_uri; } } else { // Build the content of the file. @@ -940,6 +957,21 @@ final class DiffusionBrowseFileController extends DiffusionBrowseController { return $box; } + private function buildErrorCorpus($message) { + $text = id(new PHUIBoxView()) + ->addPadding(PHUI::PADDING_LARGE) + ->appendChild($message); + + $header = id(new PHUIHeaderView()) + ->setHeader(pht('Details')); + + $box = id(new PHUIObjectBoxView()) + ->setHeader($header) + ->appendChild($text); + + return $box; + } + private function buildBeforeResponse($before) { $request = $this->getRequest(); $drequest = $this->getDiffusionRequest(); diff --git a/src/applications/diffusion/query/filecontent/DiffusionFileContentQuery.php b/src/applications/diffusion/query/filecontent/DiffusionFileContentQuery.php index 2f3493737a..9664d98f07 100644 --- a/src/applications/diffusion/query/filecontent/DiffusionFileContentQuery.php +++ b/src/applications/diffusion/query/filecontent/DiffusionFileContentQuery.php @@ -11,6 +11,26 @@ abstract class DiffusionFileContentQuery extends DiffusionQuery { private $needsBlame; private $fileContent; private $viewer; + private $timeout; + private $byteLimit; + + public function setTimeout($timeout) { + $this->timeout = $timeout; + return $this; + } + + public function getTimeout() { + return $this->timeout; + } + + public function setByteLimit($byte_limit) { + $this->byteLimit = $byte_limit; + return $this; + } + + public function getByteLimit() { + return $this->byteLimit; + } final public static function newFromDiffusionRequest( DiffusionRequest $request) { @@ -21,7 +41,31 @@ abstract class DiffusionFileContentQuery extends DiffusionQuery { abstract protected function executeQueryFromFuture(Future $future); final public function loadFileContentFromFuture(Future $future) { - $this->fileContent = $this->executeQueryFromFuture($future); + + if ($this->timeout) { + $future->setTimeout($this->timeout); + } + + if ($this->getByteLimit()) { + $future->setStdoutSizeLimit($this->getByteLimit()); + } + + try { + $file_content = $this->executeQueryFromFuture($future); + } catch (CommandException $ex) { + if (!$future->getWasKilledByTimeout()) { + throw $ex; + } + + $message = pht( + '', + $this->timeout); + + $file_content = new DiffusionFileContent(); + $file_content->setCorpus($message); + } + + $this->fileContent = $file_content; $repository = $this->getRequest()->getRepository(); $try_encoding = $repository->getDetail('encoding');