Aaron Schulz has uploaded a new change for review. https://gerrit.wikimedia.org/r/273561
Change subject: Allow FSFile objects for src in FileBackend::do*Operations ...................................................................... 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. Change-Id: I1254de527c47835c35fed6e526b42953c1b2b2ca --- M includes/filebackend/FileBackend.php M includes/filebackend/FileBackendMultiWrite.php M includes/filerepo/FileRepo.php 3 files changed, 52 insertions(+), 13 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/mediawiki/core refs/changes/61/273561/1 diff --git a/includes/filebackend/FileBackend.php b/includes/filebackend/FileBackend.php index fd9c8de..03974f7 100644 --- a/includes/filebackend/FileBackend.php +++ b/includes/filebackend/FileBackend.php @@ -237,6 +237,8 @@ * - 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 @@ * @code * array( * 'op' => 'store', - * 'src' => <file system path>, + * 'src' => <file system path, FSFile, or TempFSFile>, * 'dst' => <storage path>, * 'overwrite' => <boolean>, * 'overwriteSame' => <boolean>, @@ -372,11 +374,15 @@ 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 @@ * - 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 @@ * @code * array( * 'op' => 'store', - * 'src' => <file system path>, + * 'src' => <file system path, FSFile, or TempFSFile>, * 'dst' => <storage path>, * 'headers' => <HTTP header name/value map> # since 1.21 * ) @@ -604,11 +612,15 @@ 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 ); } @@ -1331,6 +1343,28 @@ } /** + * 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 2d27450..5a103c6 100644 --- a/includes/filebackend/FileBackendMultiWrite.php +++ b/includes/filebackend/FileBackendMultiWrite.php @@ -199,7 +199,7 @@ } $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 @@ } /** - * @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 @@ } $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 24c3964..789803f 100644 --- a/includes/filerepo/FileRepo.php +++ b/includes/filerepo/FileRepo.php @@ -956,7 +956,7 @@ * 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 @@ * 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 @@ $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 @@ } $operations[] = [ - 'op' => FileBackend::isStoragePath( $src ) ? 'copy' : 'store', + 'op' => $op, 'src' => $src, 'dst' => $dst, 'headers' => $headers -- To view, visit https://gerrit.wikimedia.org/r/273561 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I1254de527c47835c35fed6e526b42953c1b2b2ca Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/core Gerrit-Branch: master Gerrit-Owner: Aaron Schulz <asch...@wikimedia.org> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits