From be00264ae74b12c0da7a6b1d27468774d103fc05 Mon Sep 17 00:00:00 2001 From: Austin McKinley Date: Tue, 18 Apr 2017 11:07:24 -0700 Subject: [PATCH] Make daemons perform file deletion Summary: Deletion is a possibly time-intensive process, especially with large files that are backed by high-latency, chunked storage (such as S3). Even ~200mb objects take minutes to delete, which makes for an unhappy experience. Fixes T10828. Test Plan: Delete a large file, and stare in awe of the swiftness with which I am redirected to the main file application. Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: thoughtpolice, Korvin Maniphest Tasks: T10828 Differential Revision: https://secure.phabricator.com/D15743 --- .../autopatches/20170418.files.isDeleted.sql | 2 ++ src/__phutil_library_map__.php | 2 ++ .../PhabricatorFileDataController.php | 1 + .../PhabricatorFileDeleteController.php | 12 +++++-- .../PhabricatorFileInfoController.php | 2 ++ .../editor/PhabricatorFileEditEngine.php | 4 ++- .../files/query/PhabricatorFileQuery.php | 13 ++++++++ .../query/PhabricatorFileSearchEngine.php | 4 ++- .../files/storage/PhabricatorFile.php | 2 ++ .../files/worker/FileDeletionWorker.php | 31 +++++++++++++++++++ 10 files changed, 69 insertions(+), 4 deletions(-) create mode 100644 resources/sql/autopatches/20170418.files.isDeleted.sql create mode 100644 src/applications/files/worker/FileDeletionWorker.php diff --git a/resources/sql/autopatches/20170418.files.isDeleted.sql b/resources/sql/autopatches/20170418.files.isDeleted.sql new file mode 100644 index 0000000000..1349e3cbc7 --- /dev/null +++ b/resources/sql/autopatches/20170418.files.isDeleted.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_file.file + ADD isDeleted BOOL NOT NULL DEFAULT 0; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index eb71aaacd3..58a963b792 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1098,6 +1098,7 @@ phutil_register_library_map(array( 'FileAllocateConduitAPIMethod' => 'applications/files/conduit/FileAllocateConduitAPIMethod.php', 'FileConduitAPIMethod' => 'applications/files/conduit/FileConduitAPIMethod.php', 'FileCreateMailReceiver' => 'applications/files/mail/FileCreateMailReceiver.php', + 'FileDeletionWorker' => 'applications/files/worker/FileDeletionWorker.php', 'FileDownloadConduitAPIMethod' => 'applications/files/conduit/FileDownloadConduitAPIMethod.php', 'FileInfoConduitAPIMethod' => 'applications/files/conduit/FileInfoConduitAPIMethod.php', 'FileMailReceiver' => 'applications/files/mail/FileMailReceiver.php', @@ -5977,6 +5978,7 @@ phutil_register_library_map(array( 'FileAllocateConduitAPIMethod' => 'FileConduitAPIMethod', 'FileConduitAPIMethod' => 'ConduitAPIMethod', 'FileCreateMailReceiver' => 'PhabricatorMailReceiver', + 'FileDeletionWorker' => 'PhabricatorWorker', 'FileDownloadConduitAPIMethod' => 'FileConduitAPIMethod', 'FileInfoConduitAPIMethod' => 'FileConduitAPIMethod', 'FileMailReceiver' => 'PhabricatorObjectMailReceiver', diff --git a/src/applications/files/controller/PhabricatorFileDataController.php b/src/applications/files/controller/PhabricatorFileDataController.php index 31761d1244..c98c05e27b 100644 --- a/src/applications/files/controller/PhabricatorFileDataController.php +++ b/src/applications/files/controller/PhabricatorFileDataController.php @@ -143,6 +143,7 @@ final class PhabricatorFileDataController extends PhabricatorFileController { $file = id(new PhabricatorFileQuery()) ->setViewer($viewer) ->withPHIDs(array($this->phid)) + ->withIsDeleted(false) ->executeOne(); if (!$file) { diff --git a/src/applications/files/controller/PhabricatorFileDeleteController.php b/src/applications/files/controller/PhabricatorFileDeleteController.php index 1077625bb1..3c64e11a8c 100644 --- a/src/applications/files/controller/PhabricatorFileDeleteController.php +++ b/src/applications/files/controller/PhabricatorFileDeleteController.php @@ -9,6 +9,7 @@ final class PhabricatorFileDeleteController extends PhabricatorFileController { $file = id(new PhabricatorFileQuery()) ->setViewer($viewer) ->withIDs(array($id)) + ->withIsDeleted(false) ->requireCapabilities( array( PhabricatorPolicyCapability::CAN_VIEW, @@ -25,8 +26,15 @@ final class PhabricatorFileDeleteController extends PhabricatorFileController { } if ($request->isFormPost()) { - $engine = new PhabricatorDestructionEngine(); - $engine->destroyObject($file); + // Mark the file for deletion, save it, and schedule a worker to + // sweep by later and pick it up. + $file->setIsDeleted(true)->save(); + + PhabricatorWorker::scheduleTask( + 'FileDeletionWorker', + array('objectPHID' => $file->getPHID()), + array('priority' => PhabricatorWorker::PRIORITY_BULK)); + return id(new AphrontRedirectResponse())->setURI('/file/'); } diff --git a/src/applications/files/controller/PhabricatorFileInfoController.php b/src/applications/files/controller/PhabricatorFileInfoController.php index f7e72be2aa..061790aa31 100644 --- a/src/applications/files/controller/PhabricatorFileInfoController.php +++ b/src/applications/files/controller/PhabricatorFileInfoController.php @@ -15,6 +15,7 @@ final class PhabricatorFileInfoController extends PhabricatorFileController { $file = id(new PhabricatorFileQuery()) ->setViewer($viewer) ->withPHIDs(array($phid)) + ->withIsDeleted(false) ->executeOne(); if (!$file) { @@ -25,6 +26,7 @@ final class PhabricatorFileInfoController extends PhabricatorFileController { $file = id(new PhabricatorFileQuery()) ->setViewer($viewer) ->withIDs(array($id)) + ->withIsDeleted(false) ->executeOne(); if (!$file) { return new Aphront404Response(); diff --git a/src/applications/files/editor/PhabricatorFileEditEngine.php b/src/applications/files/editor/PhabricatorFileEditEngine.php index 04c4753bd5..9c5eaef74a 100644 --- a/src/applications/files/editor/PhabricatorFileEditEngine.php +++ b/src/applications/files/editor/PhabricatorFileEditEngine.php @@ -36,7 +36,9 @@ final class PhabricatorFileEditEngine } protected function newObjectQuery() { - return new PhabricatorFileQuery(); + $query = new PhabricatorFileQuery(); + $query->withIsDeleted(false); + return $query; } protected function getObjectCreateTitleText($object) { diff --git a/src/applications/files/query/PhabricatorFileQuery.php b/src/applications/files/query/PhabricatorFileQuery.php index 6439f75405..a2de5ca6ab 100644 --- a/src/applications/files/query/PhabricatorFileQuery.php +++ b/src/applications/files/query/PhabricatorFileQuery.php @@ -15,6 +15,7 @@ final class PhabricatorFileQuery private $maxLength; private $names; private $isPartial; + private $isDeleted; private $needTransforms; private $builtinKeys; @@ -119,6 +120,11 @@ final class PhabricatorFileQuery return $this; } + public function withIsDeleted($deleted) { + $this->isDeleted = $deleted; + return $this; + } + public function withNameNgrams($ngrams) { return $this->withNgramsConstraint( id(new PhabricatorFileNameNgrams()), @@ -396,6 +402,13 @@ final class PhabricatorFileQuery (int)$this->isPartial); } + if ($this->isDeleted !== null) { + $where[] = qsprintf( + $conn, + 'isDeleted = %d', + (int)$this->isDeleted); + } + if ($this->builtinKeys !== null) { $where[] = qsprintf( $conn, diff --git a/src/applications/files/query/PhabricatorFileSearchEngine.php b/src/applications/files/query/PhabricatorFileSearchEngine.php index 0d989000cf..59810c830c 100644 --- a/src/applications/files/query/PhabricatorFileSearchEngine.php +++ b/src/applications/files/query/PhabricatorFileSearchEngine.php @@ -16,7 +16,9 @@ final class PhabricatorFileSearchEngine } public function newQuery() { - return new PhabricatorFileQuery(); + $query = new PhabricatorFileQuery(); + $query->withIsDeleted(false); + return $query; } protected function buildCustomSearchFields() { diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php index 1c9b96069d..7d314d213d 100644 --- a/src/applications/files/storage/PhabricatorFile.php +++ b/src/applications/files/storage/PhabricatorFile.php @@ -59,6 +59,7 @@ final class PhabricatorFile extends PhabricatorFileDAO protected $isExplicitUpload = 1; protected $viewPolicy = PhabricatorPolicies::POLICY_USER; protected $isPartial = 0; + protected $isDeleted = 0; private $objects = self::ATTACHABLE; private $objectPHIDs = self::ATTACHABLE; @@ -103,6 +104,7 @@ final class PhabricatorFile extends PhabricatorFileDAO 'mailKey' => 'bytes20', 'isPartial' => 'bool', 'builtinKey' => 'text64?', + 'isDeleted' => 'bool', ), self::CONFIG_KEY_SCHEMA => array( 'key_phid' => null, diff --git a/src/applications/files/worker/FileDeletionWorker.php b/src/applications/files/worker/FileDeletionWorker.php new file mode 100644 index 0000000000..0cc89ac30e --- /dev/null +++ b/src/applications/files/worker/FileDeletionWorker.php @@ -0,0 +1,31 @@ +getTaskData(), 'objectPHID'); + if (!$phid) { + throw new PhabricatorWorkerPermanentFailureException( + pht('No "%s" in task data.', 'objectPHID')); + } + + $file = id(new PhabricatorFileQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withPHIDs(array($phid)) + ->executeOne(); + + if (!$file) { + throw new PhabricatorWorkerPermanentFailureException( + pht('File "%s" does not exist.', $phid)); + } + + return $file; + } + + protected function doWork() { + $file = $this->loadFile(); + $engine = new PhabricatorDestructionEngine(); + $engine->destroyObject($file); + } + +}