From 81d88985a027213f247898850943dcc7a9fe2314 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 14 Mar 2015 08:29:12 -0700 Subject: [PATCH] Prepare file responses for streaming chunks Summary: Ref T7149. This still buffers the whole file, but is reaaaaal close to not doing that. Allow Responses to be streamed, and rewrite the range stuff in the FileResponse so it does not rely on having the entire content available. Test Plan: - Artificially slowed down downloads, suspended/resumed them (works in chrome, not so much in Safari/Firefox?) - Played sounds in Safari/Chrome. - Viewed a bunch of pages and files in every browser. Reviewers: btrahan Reviewed By: btrahan Subscribers: joshuaspence, epriestley Maniphest Tasks: T7149 Differential Revision: https://secure.phabricator.com/D12072 --- src/aphront/response/AphrontFileResponse.php | 63 ++++++++++++++++--- src/aphront/response/AphrontResponse.php | 20 +++++- src/aphront/sink/AphrontHTTPSink.php | 19 +++++- src/aphront/sink/AphrontIsolatedHTTPSink.php | 4 ++ src/aphront/sink/AphrontPHPHTTPSink.php | 9 +++ .../PhabricatorFileDataController.php | 24 ++++--- .../query/PhabricatorFileSearchEngine.php | 8 ++- 7 files changed, 127 insertions(+), 20 deletions(-) diff --git a/src/aphront/response/AphrontFileResponse.php b/src/aphront/response/AphrontFileResponse.php index 9e5bc50b48..bf58421c68 100644 --- a/src/aphront/response/AphrontFileResponse.php +++ b/src/aphront/response/AphrontFileResponse.php @@ -3,11 +3,15 @@ final class AphrontFileResponse extends AphrontResponse { private $content; + private $contentIterator; + private $contentLength; + private $mimeType; private $download; private $rangeMin; private $rangeMax; private $allowOrigins = array(); + private $fileToken; public function addAllowOrigin($origin) { $this->allowOrigins[] = $origin; @@ -36,17 +40,34 @@ final class AphrontFileResponse extends AphrontResponse { } public function setContent($content) { + $this->setContentLength(strlen($content)); $this->content = $content; return $this; } + public function setContentIterator($iterator) { + $this->contentIterator = $iterator; + return $this; + } + public function buildResponseString() { - if ($this->rangeMin || $this->rangeMax) { - $length = ($this->rangeMax - $this->rangeMin) + 1; - return substr($this->content, $this->rangeMin, $length); - } else { - return $this->content; + return $this->content; + } + + public function getContentIterator() { + if ($this->contentIterator) { + return $this->contentIterator; } + return parent::getContentIterator(); + } + + public function setContentLength($length) { + $this->contentLength = $length; + return $this; + } + + public function getContentLength() { + return $this->contentLength; } public function setRange($min, $max) { @@ -55,19 +76,36 @@ final class AphrontFileResponse extends AphrontResponse { return $this; } + public function setTemporaryFileToken(PhabricatorAuthTemporaryToken $token) { + $this->fileToken = $token; + return $this; + } + + public function getTemporaryFileToken() { + return $this->fileToken; + } + public function getHeaders() { $headers = array( array('Content-Type', $this->getMimeType()), - array('Content-Length', strlen($this->buildResponseString())), + // This tells clients that we can support requests with a "Range" header, + // which allows downloads to be resumed, in some browsers, some of the + // time, if the stars align. + array('Accept-Ranges', 'bytes'), ); if ($this->rangeMin || $this->rangeMax) { - $len = strlen($this->content); + $len = $this->getContentLength(); $min = $this->rangeMin; $max = $this->rangeMax; $headers[] = array('Content-Range', "bytes {$min}-{$max}/{$len}"); + $content_len = ($max - $min) + 1; + } else { + $content_len = $this->getContentLength(); } + $headers[] = array('Content-Length', $this->getContentLength()); + if (strlen($this->getDownload())) { $headers[] = array('X-Download-Options', 'noopen'); @@ -90,4 +128,15 @@ final class AphrontFileResponse extends AphrontResponse { return $headers; } + public function didCompleteWrite($aborted) { + if (!$aborted) { + $token = $this->getTemporaryFileToken(); + if ($token) { + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + $token->delete(); + unset($unguarded); + } + } + } + } diff --git a/src/aphront/response/AphrontResponse.php b/src/aphront/response/AphrontResponse.php index 27c603b091..4cf420f066 100644 --- a/src/aphront/response/AphrontResponse.php +++ b/src/aphront/response/AphrontResponse.php @@ -18,6 +18,22 @@ abstract class AphrontResponse { return $this->request; } + +/* -( Content )------------------------------------------------------------ */ + + + public function getContentIterator() { + return array($this->buildResponseString()); + } + + public function buildResponseString() { + throw new PhutilMethodNotImplementedException(); + } + + +/* -( Metadata )----------------------------------------------------------- */ + + public function getHeaders() { $headers = array(); if (!$this->frameable) { @@ -165,6 +181,8 @@ abstract class AphrontResponse { return gmdate('D, d M Y H:i:s', $epoch_timestamp).' GMT'; } - abstract public function buildResponseString(); + public function didCompleteWrite($aborted) { + return; + } } diff --git a/src/aphront/sink/AphrontHTTPSink.php b/src/aphront/sink/AphrontHTTPSink.php index 9c6efca4f6..72bf4244f9 100644 --- a/src/aphront/sink/AphrontHTTPSink.php +++ b/src/aphront/sink/AphrontHTTPSink.php @@ -94,8 +94,10 @@ abstract class AphrontHTTPSink { * @return void */ final public function writeResponse(AphrontResponse $response) { - // Do this first, in case it throws. - $response_string = $response->buildResponseString(); + // Build the content iterator first, in case it throws. Ideally, we'd + // prefer to handle exceptions before we emit the response status or any + // HTTP headers. + $data = $response->getContentIterator(); $all_headers = array_merge( $response->getHeaders(), @@ -105,7 +107,17 @@ abstract class AphrontHTTPSink { $response->getHTTPResponseCode(), $response->getHTTPResponseMessage()); $this->writeHeaders($all_headers); - $this->writeData($response_string); + + $abort = false; + foreach ($data as $block) { + if (!$this->isWritable()) { + $abort = true; + break; + } + $this->writeData($block); + } + + $response->didCompleteWrite($abort); } @@ -115,5 +127,6 @@ abstract class AphrontHTTPSink { abstract protected function emitHTTPStatus($code, $message = ''); abstract protected function emitHeader($name, $value); abstract protected function emitData($data); + abstract protected function isWritable(); } diff --git a/src/aphront/sink/AphrontIsolatedHTTPSink.php b/src/aphront/sink/AphrontIsolatedHTTPSink.php index ac49b1f9bc..0b0c2b1685 100644 --- a/src/aphront/sink/AphrontIsolatedHTTPSink.php +++ b/src/aphront/sink/AphrontIsolatedHTTPSink.php @@ -21,6 +21,10 @@ final class AphrontIsolatedHTTPSink extends AphrontHTTPSink { $this->data .= $data; } + protected function isWritable() { + return true; + } + public function getEmittedHTTPStatus() { return $this->status; } diff --git a/src/aphront/sink/AphrontPHPHTTPSink.php b/src/aphront/sink/AphrontPHPHTTPSink.php index f028340fa9..2504aea037 100644 --- a/src/aphront/sink/AphrontPHPHTTPSink.php +++ b/src/aphront/sink/AphrontPHPHTTPSink.php @@ -21,6 +21,15 @@ final class AphrontPHPHTTPSink extends AphrontHTTPSink { protected function emitData($data) { echo $data; + + // Try to push the data to the browser. This has a lot of caveats around + // browser buffering and display behavior, but approximately works most + // of the time. + flush(); + } + + protected function isWritable() { + return !connection_aborted(); } } diff --git a/src/applications/files/controller/PhabricatorFileDataController.php b/src/applications/files/controller/PhabricatorFileDataController.php index 6d793fa7b3..2a66a3807e 100644 --- a/src/applications/files/controller/PhabricatorFileDataController.php +++ b/src/applications/files/controller/PhabricatorFileDataController.php @@ -117,13 +117,14 @@ final class PhabricatorFileDataController extends PhabricatorFileController { } } - $data = $file->loadFileData(); $response = new AphrontFileResponse(); - $response->setContent($data); if ($cache_response) { $response->setCacheDurationInSeconds(60 * 60 * 24 * 30); } + $begin = null; + $end = null; + // NOTE: It's important to accept "Range" requests when playing audio. // If we don't, Safari has difficulty figuring out how long sounds are // and glitches when trying to loop them. In particular, Safari sends @@ -133,14 +134,18 @@ final class PhabricatorFileDataController extends PhabricatorFileController { if ($range) { $matches = null; if (preg_match('/^bytes=(\d+)-(\d+)$/', $range, $matches)) { + // Note that the "Range" header specifies bytes differently than + // we do internally: the range 0-1 has 2 bytes (byte 0 and byte 1). + $begin = (int)$matches[1]; + $end = (int)$matches[2] + 1; + $response->setHTTPResponseCode(206); - $response->setRange((int)$matches[1], (int)$matches[2]); + $response->setRange($begin, ($end - 1)); } } else if (isset($validated_token)) { - // consume the one-time token if we have one. - $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); - $validated_token->delete(); - unset($unguarded); + // We set this on the response, and the response deletes it after the + // transfer completes. This allows transfers to be resumed, in theory. + $response->setTemporaryFileToken($validated_token); } $is_viewable = $file->isViewableInBrowser(); @@ -165,6 +170,11 @@ final class PhabricatorFileDataController extends PhabricatorFileController { $response->setDownload($file->getName()); } + $iterator = $file->getFileDataIterator($begin, $end); + + $response->setContentLength($file->getByteSize()); + $response->setContentIterator($iterator); + return $response; } diff --git a/src/applications/files/query/PhabricatorFileSearchEngine.php b/src/applications/files/query/PhabricatorFileSearchEngine.php index fa9d2332c7..0eaa711297 100644 --- a/src/applications/files/query/PhabricatorFileSearchEngine.php +++ b/src/applications/files/query/PhabricatorFileSearchEngine.php @@ -25,8 +25,12 @@ final class PhabricatorFileSearchEngine } public function buildQueryFromSavedQuery(PhabricatorSavedQuery $saved) { - $query = id(new PhabricatorFileQuery()) - ->withAuthorPHIDs($saved->getParameter('authorPHIDs', array())); + $query = id(new PhabricatorFileQuery()); + + $author_phids = $saved->getParameter('authorPHIDs', array()); + if ($author_phids) { + $query->withAuthorPHIDs($author_phids); + } if ($saved->getParameter('explicit')) { $query->showOnlyExplicitUploads(true);