From 8ad61d01502d5b95e2a131476d7071f3ff112a13 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 11 Jul 2016 09:16:03 -0700 Subject: [PATCH] Simplify "builtin file" management and recover from races Summary: Fixes T11307. Fixes T8124. Currently, builtin files are tracked by using a special transform with an invalid source ID. Just use a dedicated column instead. The transform thing is too clever/weird/hacky and exposes us to issues with the "file" and "transform" tables getting out of sync (possibly the issue in T11307?) and with race conditions. Test Plan: - Loaded profile "edit picture" page, saw builtins. - Deleted all builtin files, put 3 second sleep in the storage engine write, loaded profile page in two windows. - Before patch: one of them failed with a race. - After patch: both of them loaded. Reviewers: chad Reviewed By: chad Maniphest Tasks: T8124, T11307 Differential Revision: https://secure.phabricator.com/D16271 --- .../autopatches/20160711.files.01.builtin.sql | 2 + .../20160711.files.02.builtinkey.sql | 2 + .../files/query/PhabricatorFileQuery.php | 13 +++++++ .../files/storage/PhabricatorFile.php | 38 +++++++++++-------- 4 files changed, 40 insertions(+), 15 deletions(-) create mode 100644 resources/sql/autopatches/20160711.files.01.builtin.sql create mode 100644 resources/sql/autopatches/20160711.files.02.builtinkey.sql diff --git a/resources/sql/autopatches/20160711.files.01.builtin.sql b/resources/sql/autopatches/20160711.files.01.builtin.sql new file mode 100644 index 0000000000..d8849ec053 --- /dev/null +++ b/resources/sql/autopatches/20160711.files.01.builtin.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_file.file + ADD builtinKey VARCHAR(64) COLLATE {$COLLATE_TEXT}; diff --git a/resources/sql/autopatches/20160711.files.02.builtinkey.sql b/resources/sql/autopatches/20160711.files.02.builtinkey.sql new file mode 100644 index 0000000000..3551f6c3cd --- /dev/null +++ b/resources/sql/autopatches/20160711.files.02.builtinkey.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_file.file + ADD UNIQUE KEY `key_builtin` (builtinKey); diff --git a/src/applications/files/query/PhabricatorFileQuery.php b/src/applications/files/query/PhabricatorFileQuery.php index 5e1876acd6..c2ac083fea 100644 --- a/src/applications/files/query/PhabricatorFileQuery.php +++ b/src/applications/files/query/PhabricatorFileQuery.php @@ -16,6 +16,7 @@ final class PhabricatorFileQuery private $names; private $isPartial; private $needTransforms; + private $builtinKeys; public function withIDs(array $ids) { $this->ids = $ids; @@ -47,6 +48,11 @@ final class PhabricatorFileQuery return $this; } + public function withBuiltinKeys(array $keys) { + $this->builtinKeys = $keys; + return $this; + } + /** * Select files which are transformations of some other file. For example, * you can use this query to find previously generated thumbnails of an image @@ -384,6 +390,13 @@ final class PhabricatorFileQuery (int)$this->isPartial); } + if ($this->builtinKeys !== null) { + $where[] = qsprintf( + $conn, + 'builtinKey IN (%Ls)', + $this->builtinKeys); + } + return $where; } diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php index 7e8f4b18fc..8d81c3ae98 100644 --- a/src/applications/files/storage/PhabricatorFile.php +++ b/src/applications/files/storage/PhabricatorFile.php @@ -42,6 +42,7 @@ final class PhabricatorFile extends PhabricatorFileDAO protected $contentHash; protected $metadata = array(); protected $mailKey; + protected $builtinKey; protected $storageEngine; protected $storageFormat; @@ -94,6 +95,7 @@ final class PhabricatorFile extends PhabricatorFileDAO 'isExplicitUpload' => 'bool?', 'mailKey' => 'bytes20', 'isPartial' => 'bool', + 'builtinKey' => 'text64?', ), self::CONFIG_KEY_SCHEMA => array( 'key_phid' => null, @@ -116,6 +118,10 @@ final class PhabricatorFile extends PhabricatorFileDAO 'key_partial' => array( 'columns' => array('authorPHID', 'isPartial'), ), + 'key_builtin' => array( + 'columns' => array('builtinKey'), + 'unique' => true, + ), ), ) + parent::getConfiguration(); } @@ -1070,19 +1076,11 @@ final class PhabricatorFile extends PhabricatorFileDAO public static function loadBuiltins(PhabricatorUser $user, array $builtins) { $builtins = mpull($builtins, null, 'getBuiltinFileKey'); - $specs = array(); - foreach ($builtins as $key => $buitin) { - $specs[] = array( - 'originalPHID' => PhabricatorPHIDConstants::PHID_VOID, - 'transform' => $key, - ); - } - // NOTE: Anyone is allowed to access builtin files. $files = id(new PhabricatorFileQuery()) ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withTransforms($specs) + ->withBuiltinKeys(array_keys($builtins)) ->execute(); $results = array(); @@ -1109,12 +1107,21 @@ final class PhabricatorFile extends PhabricatorFileDAO ); $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); - $file = self::newFromFileData($data, $params); - $xform = id(new PhabricatorTransformedFile()) - ->setOriginalPHID(PhabricatorPHIDConstants::PHID_VOID) - ->setTransform($key) - ->setTransformedPHID($file->getPHID()) - ->save(); + try { + $file = self::newFromFileData($data, $params); + } catch (AphrontDuplicateKeyQueryException $ex) { + $file = id(new PhabricatorFileQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withBuiltinKeys(array($key)) + ->executeOne(); + if (!$file) { + throw new Exception( + pht( + 'Collided mid-air when generating builtin file "%s", but '. + 'then failed to load the object we collided with.', + $key)); + } + } unset($unguarded); $file->attachObjectPHIDs(array()); @@ -1289,6 +1296,7 @@ final class PhabricatorFile extends PhabricatorFileDAO $builtin = idx($params, 'builtin'); if ($builtin) { $this->setBuiltinName($builtin); + $this->setBuiltinKey($builtin); } $profile = idx($params, 'profile');