From a5d903860a14c0ca84fa2688f8cb0292a6fd1487 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Fri, 26 Feb 2016 17:33:42 -0800 Subject: [PATCH] Allow FSFile objects for src in FileBackend::do*Operations Convenience aside, this lets multiwrite backends do async replication for "store" operations safely, buy keeping a handle to the source file that prevents it from getting prematurely deleted before the post-send writes to the secondary backends can happen. Bug: T91869 Change-Id: I1254de527c47835c35fed6e526b42953c1b2b2ca --- includes/filebackend/FileBackend.php | 38 ++++++++++++++++++- .../filebackend/FileBackendMultiWrite.php | 14 +++---- includes/filerepo/FileRepo.php | 13 +++++-- 3 files changed, 52 insertions(+), 13 deletions(-) diff --git a/includes/filebackend/FileBackend.php b/includes/filebackend/FileBackend.php index fd9c8deead..03974f755a 100644 --- a/includes/filebackend/FileBackend.php +++ b/includes/filebackend/FileBackend.php @@ -237,6 +237,8 @@ abstract class FileBackend { * - describe (since 1.21) * - null * + * FSFile/TempFSFile object support was added in 1.27. + * * a) Create a new file in storage with the contents of a string * @code * array( @@ -253,7 +255,7 @@ abstract class FileBackend { * @code * array( * 'op' => 'store', - * 'src' => , + * 'src' => , * 'dst' => , * 'overwrite' => , * 'overwriteSame' => , @@ -372,11 +374,15 @@ abstract class FileBackend { if ( !count( $ops ) ) { return Status::newGood(); // nothing to do } + + $ops = $this->resolveFSFileObjects( $ops ); if ( empty( $opts['force'] ) ) { // sanity unset( $opts['nonLocking'] ); } + /** @noinspection PhpUnusedLocalVariableInspection */ $scope = $this->getScopedPHPBehaviorForOps(); // try to ignore client aborts + return $this->doOperationsInternal( $ops, $opts ); } @@ -503,6 +509,8 @@ abstract class FileBackend { * - describe (since 1.21) * - null * + * FSFile/TempFSFile object support was added in 1.27. + * * a) Create a new file in storage with the contents of a string * @code * array( @@ -517,7 +525,7 @@ abstract class FileBackend { * @code * array( * 'op' => 'store', - * 'src' => , + * 'src' => , * 'dst' => , * 'headers' => # since 1.21 * ) @@ -604,11 +612,15 @@ abstract class FileBackend { if ( !count( $ops ) ) { return Status::newGood(); // nothing to do } + + $ops = $this->resolveFSFileObjects( $ops ); foreach ( $ops as &$op ) { $op['overwrite'] = true; // avoids RTTs in key/value stores } + /** @noinspection PhpUnusedLocalVariableInspection */ $scope = $this->getScopedPHPBehaviorForOps(); // try to ignore client aborts + return $this->doQuickOperationsInternal( $ops ); } @@ -1330,6 +1342,28 @@ abstract class FileBackend { return $this->fileJournal; } + /** + * Convert FSFile 'src' paths to string paths (with an 'srcRef' field set to the FSFile) + * + * The 'srcRef' field keeps any TempFSFile objects in scope for the backend to have it + * around as long it needs (which may vary greatly depending on configuration) + * + * @param array $ops File operation batch for FileBaclend::doOperations() + * @return array File operation batch + */ + protected function resolveFSFileObjects( array $ops ) { + foreach ( $ops as &$op ) { + $src = isset( $op['src'] ) ? $op['src'] : null; + if ( $src instanceof FSFile ) { + $op['srcRef'] = $src; + $op['src'] = $src->getPath(); + } + } + unset( $op ); + + return $ops; + } + /** * Check if a given path is a "mwstore://" path. * This does not do any further validation or any existence checks. diff --git a/includes/filebackend/FileBackendMultiWrite.php b/includes/filebackend/FileBackendMultiWrite.php index 2d2745097f..5a103c647b 100644 --- a/includes/filebackend/FileBackendMultiWrite.php +++ b/includes/filebackend/FileBackendMultiWrite.php @@ -199,7 +199,7 @@ class FileBackendMultiWrite extends FileBackend { } $realOps = $this->substOpBatchPaths( $ops, $backend ); - if ( $this->asyncWrites && !$this->hasStoreOperation( $ops ) ) { + if ( $this->asyncWrites && !$this->hasVolatileSources( $ops ) ) { // Bind $scopeLock to the callback to preserve locks DeferredUpdates::addCallableUpdate( function() use ( $backend, $realOps, $opts, $scopeLock ) { @@ -468,13 +468,13 @@ class FileBackendMultiWrite extends FileBackend { } /** - * @param array $ops File operation batch map - * @return bool + * @param array $ops File operations for FileBackend::doOperations() + * @return bool Whether there are file path sources with outside lifetime/ownership */ - protected function hasStoreOperation( array $ops ) { + protected function hasVolatileSources( array $ops ) { foreach ( $ops as $op ) { - if ( $op['op'] === 'store' ) { - return true; + if ( $op['op'] === 'store' && !isset( $op['srcRef'] ) ) { + return true; // source file might be deleted anytime after do*Operations() } } @@ -494,7 +494,7 @@ class FileBackendMultiWrite extends FileBackend { } $realOps = $this->substOpBatchPaths( $ops, $backend ); - if ( $this->asyncWrites && !$this->hasStoreOperation( $ops ) ) { + if ( $this->asyncWrites && !$this->hasVolatileSources( $ops ) ) { DeferredUpdates::addCallableUpdate( function() use ( $backend, $realOps ) { $backend->doQuickOperations( $realOps ); diff --git a/includes/filerepo/FileRepo.php b/includes/filerepo/FileRepo.php index 24c3964a98..789803f616 100644 --- a/includes/filerepo/FileRepo.php +++ b/includes/filerepo/FileRepo.php @@ -956,7 +956,7 @@ class FileRepo { * This function can be used to write to otherwise read-only foreign repos. * This is intended for copying generated thumbnails into the repo. * - * @param string $src Source file system path, storage path, or virtual URL + * @param string|FSFile $src Source file system path, storage path, or virtual URL * @param string $dst Virtual URL or storage path * @param array|string|null $options An array consisting of a key named headers * listing extra headers. If a string, taken as content-disposition header. @@ -1003,7 +1003,7 @@ class FileRepo { * All path parameters may be a file system path, storage path, or virtual URL. * When "headers" are given they are used as HTTP headers if supported. * - * @param array $triples List of (source path, destination path, disposition) + * @param array $triples List of (source path or FSFile, destination path, disposition) * @return FileRepoStatus */ public function quickImportBatch( array $triples ) { @@ -1011,7 +1011,12 @@ class FileRepo { $operations = []; foreach ( $triples as $triple ) { list( $src, $dst ) = $triple; - $src = $this->resolveToStoragePath( $src ); + if ( $src instanceof FSFile ) { + $op = 'store'; + } else { + $src = $this->resolveToStoragePath( $src ); + $op = FileBackend::isStoragePath( $src ) ? 'copy' : 'store'; + } $dst = $this->resolveToStoragePath( $dst ); if ( !isset( $triple[2] ) ) { @@ -1026,7 +1031,7 @@ class FileRepo { } $operations[] = [ - 'op' => FileBackend::isStoragePath( $src ) ? 'copy' : 'store', + 'op' => $op, 'src' => $src, 'dst' => $dst, 'headers' => $headers -- 2.20.1