1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-18 19:40:55 +01:00

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
This commit is contained in:
epriestley 2016-07-11 09:16:03 -07:00
parent 830f3eb8f8
commit 8ad61d0150
4 changed files with 40 additions and 15 deletions

View file

@ -0,0 +1,2 @@
ALTER TABLE {$NAMESPACE}_file.file
ADD builtinKey VARCHAR(64) COLLATE {$COLLATE_TEXT};

View file

@ -0,0 +1,2 @@
ALTER TABLE {$NAMESPACE}_file.file
ADD UNIQUE KEY `key_builtin` (builtinKey);

View file

@ -16,6 +16,7 @@ final class PhabricatorFileQuery
private $names; private $names;
private $isPartial; private $isPartial;
private $needTransforms; private $needTransforms;
private $builtinKeys;
public function withIDs(array $ids) { public function withIDs(array $ids) {
$this->ids = $ids; $this->ids = $ids;
@ -47,6 +48,11 @@ final class PhabricatorFileQuery
return $this; return $this;
} }
public function withBuiltinKeys(array $keys) {
$this->builtinKeys = $keys;
return $this;
}
/** /**
* Select files which are transformations of some other file. For example, * Select files which are transformations of some other file. For example,
* you can use this query to find previously generated thumbnails of an image * you can use this query to find previously generated thumbnails of an image
@ -384,6 +390,13 @@ final class PhabricatorFileQuery
(int)$this->isPartial); (int)$this->isPartial);
} }
if ($this->builtinKeys !== null) {
$where[] = qsprintf(
$conn,
'builtinKey IN (%Ls)',
$this->builtinKeys);
}
return $where; return $where;
} }

View file

@ -42,6 +42,7 @@ final class PhabricatorFile extends PhabricatorFileDAO
protected $contentHash; protected $contentHash;
protected $metadata = array(); protected $metadata = array();
protected $mailKey; protected $mailKey;
protected $builtinKey;
protected $storageEngine; protected $storageEngine;
protected $storageFormat; protected $storageFormat;
@ -94,6 +95,7 @@ final class PhabricatorFile extends PhabricatorFileDAO
'isExplicitUpload' => 'bool?', 'isExplicitUpload' => 'bool?',
'mailKey' => 'bytes20', 'mailKey' => 'bytes20',
'isPartial' => 'bool', 'isPartial' => 'bool',
'builtinKey' => 'text64?',
), ),
self::CONFIG_KEY_SCHEMA => array( self::CONFIG_KEY_SCHEMA => array(
'key_phid' => null, 'key_phid' => null,
@ -116,6 +118,10 @@ final class PhabricatorFile extends PhabricatorFileDAO
'key_partial' => array( 'key_partial' => array(
'columns' => array('authorPHID', 'isPartial'), 'columns' => array('authorPHID', 'isPartial'),
), ),
'key_builtin' => array(
'columns' => array('builtinKey'),
'unique' => true,
),
), ),
) + parent::getConfiguration(); ) + parent::getConfiguration();
} }
@ -1070,19 +1076,11 @@ final class PhabricatorFile extends PhabricatorFileDAO
public static function loadBuiltins(PhabricatorUser $user, array $builtins) { public static function loadBuiltins(PhabricatorUser $user, array $builtins) {
$builtins = mpull($builtins, null, 'getBuiltinFileKey'); $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. // NOTE: Anyone is allowed to access builtin files.
$files = id(new PhabricatorFileQuery()) $files = id(new PhabricatorFileQuery())
->setViewer(PhabricatorUser::getOmnipotentUser()) ->setViewer(PhabricatorUser::getOmnipotentUser())
->withTransforms($specs) ->withBuiltinKeys(array_keys($builtins))
->execute(); ->execute();
$results = array(); $results = array();
@ -1109,12 +1107,21 @@ final class PhabricatorFile extends PhabricatorFileDAO
); );
$unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
$file = self::newFromFileData($data, $params); try {
$xform = id(new PhabricatorTransformedFile()) $file = self::newFromFileData($data, $params);
->setOriginalPHID(PhabricatorPHIDConstants::PHID_VOID) } catch (AphrontDuplicateKeyQueryException $ex) {
->setTransform($key) $file = id(new PhabricatorFileQuery())
->setTransformedPHID($file->getPHID()) ->setViewer(PhabricatorUser::getOmnipotentUser())
->save(); ->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); unset($unguarded);
$file->attachObjectPHIDs(array()); $file->attachObjectPHIDs(array());
@ -1289,6 +1296,7 @@ final class PhabricatorFile extends PhabricatorFileDAO
$builtin = idx($params, 'builtin'); $builtin = idx($params, 'builtin');
if ($builtin) { if ($builtin) {
$this->setBuiltinName($builtin); $this->setBuiltinName($builtin);
$this->setBuiltinKey($builtin);
} }
$profile = idx($params, 'profile'); $profile = idx($params, 'profile');