From 00e44200209c88118d87a7a92c7f548d8da62472 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Fri, 6 Sep 2019 23:21:48 -0700 Subject: [PATCH] filebackend: optimize 'delete' for FSFileBackend to avoid is_file() calls This also should reduce the chance of warnings due to race conditions Change-Id: I06b6bcc5e0f009bb3d5135591d13ff098710a5b3 --- includes/libs/filebackend/FSFileBackend.php | 73 +++++++++---------- .../includes/filebackend/FileBackendTest.php | 11 +-- 2 files changed, 42 insertions(+), 42 deletions(-) diff --git a/includes/libs/filebackend/FSFileBackend.php b/includes/libs/filebackend/FSFileBackend.php index 14bf616c26..3256f93b40 100644 --- a/includes/libs/filebackend/FSFileBackend.php +++ b/includes/libs/filebackend/FSFileBackend.php @@ -66,11 +66,10 @@ class FSFileBackend extends FileBackendStore { /** @var array Map of container names to root paths for custom container paths */ protected $containerPaths; + /** @var int Directory permission mode */ + protected $dirMode; /** @var int File permission mode */ protected $fileMode; - /** @var int File permission mode */ - protected $dirMode; - /** @var string Required OS username to own files */ protected $fileOwner; @@ -79,8 +78,8 @@ class FSFileBackend extends FileBackendStore { /** @var string OS username running this script */ protected $currentUser; - /** @var array */ - protected $hadWarningErrors = []; + /** @var bool[] Map of (stack index => whether a warning happened) */ + private $warningTrapStack = []; /** * @see FileBackendStore::__construct() @@ -420,26 +419,25 @@ class FSFileBackend extends FileBackendStore { protected function doDeleteInternal( array $params ) { $status = $this->newStatus(); - $source = $this->resolveToFSPath( $params['src'] ); - if ( $source === null ) { + $fsSrcPath = $this->resolveToFSPath( $params['src'] ); + if ( $fsSrcPath === null ) { $status->fatal( 'backend-fail-invalidpath', $params['src'] ); return $status; } - if ( !is_file( $source ) ) { - if ( empty( $params['ignoreMissingSource'] ) ) { - $status->fatal( 'backend-fail-delete', $params['src'] ); - } - - return $status; // do nothing; either OK or bad status - } + $ignoreMissing = !empty( $params['ignoreMissingSource'] ); if ( !empty( $params['async'] ) ) { // deferred - $cmd = implode( ' ', [ - $this->isWindows ? 'DEL' : 'unlink', - escapeshellarg( $this->cleanPathSlashes( $source ) ) - ] ); + // https://manpages.debian.org/buster/coreutils/rm.1.en.html + // https://docs.microsoft.com/en-us/windows-server/administration/windows-commands/del + $encSrc = escapeshellarg( $this->cleanPathSlashes( $fsSrcPath ) ); + if ( $this->isWindows ) { + $writeCmd = "DEL /Q $encSrc"; + $cmd = $ignoreMissing ? "IF EXIST $encSrc $writeCmd" : $writeCmd; + } else { + $cmd = $ignoreMissing ? "rm -f $encSrc" : "rm $encSrc"; + } $handler = function ( $errors, StatusValue $status, array $params, $cmd ) { if ( $errors !== '' && !( $this->isWindows && $errors[0] === " " ) ) { $status->fatal( 'backend-fail-delete', $params['src'] ); @@ -448,8 +446,10 @@ class FSFileBackend extends FileBackendStore { }; $status->value = new FSFileOpHandle( $this, $params, $handler, $cmd ); } else { // immediate write - $ok = $this->unlink( $source ); - if ( !$ok ) { + $this->trapWarnings( '/: No such file or directory$/' ); + $deleted = unlink( $fsSrcPath ); + $hadError = $this->untrapWarnings(); + if ( $hadError || ( !$deleted && !$ignoreMissing ) ) { $status->fatal( 'backend-fail-delete', $params['src'] ); return $status; @@ -709,7 +709,7 @@ class FSFileBackend extends FileBackendStore { $tmpPath = $tmpFile->getPath(); // Copy the source file over the temp file - $this->trapWarnings(); + $this->trapWarnings(); // don't trust 'false' if there were errors $isFile = is_file( $source ); // regular files only $copySuccess = $isFile ? copy( $source, $tmpPath ) : false; $hadError = $this->untrapWarnings(); @@ -857,30 +857,29 @@ class FSFileBackend extends FileBackendStore { } /** - * Listen for E_WARNING errors and track whether any happen + * Listen for E_WARNING errors and track whether any that happen + * + * @param string|null $regexIgnore Optional regex of errors to ignore */ - protected function trapWarnings() { - // push to stack - $this->hadWarningErrors[] = false; - set_error_handler( function ( $errno, $errstr ) { - // more detailed error logging - $this->logger->error( $errstr ); - $this->hadWarningErrors[count( $this->hadWarningErrors ) - 1] = true; - - // suppress from PHP handler - return true; + protected function trapWarnings( $regexIgnore = null ) { + $this->warningTrapStack[] = false; + set_error_handler( function ( $errno, $errstr ) use ( $regexIgnore ) { + if ( $regexIgnore === null || !preg_match( $regexIgnore, $errstr ) ) { + $this->logger->error( $errstr ); + $this->warningTrapStack[count( $this->warningTrapStack ) - 1] = true; + } + return true; // suppress from PHP handler }, E_WARNING ); } /** - * Stop listening for E_WARNING errors and return true if any happened + * Stop listening for E_WARNING errors and get whether any happened * - * @return bool + * @return bool Whether any warnings happened */ protected function untrapWarnings() { - // restore previous handler restore_error_handler(); - // pop from stack - return array_pop( $this->hadWarningErrors ); + + return array_pop( $this->warningTrapStack ); } } diff --git a/tests/phpunit/includes/filebackend/FileBackendTest.php b/tests/phpunit/includes/filebackend/FileBackendTest.php index 062087da05..ae46aa7ec2 100644 --- a/tests/phpunit/includes/filebackend/FileBackendTest.php +++ b/tests/phpunit/includes/filebackend/FileBackendTest.php @@ -943,7 +943,7 @@ class FileBackendTest extends MediaWikiTestCase { "$base/unittest-cont1/e/fileC.a" ]; $createOps = []; - $purgeOps = []; + $delOps = []; foreach ( $files as $path ) { $status = $this->prepare( [ 'dir' => dirname( $path ) ] ); $this->assertGoodStatus( $status, @@ -951,10 +951,11 @@ class FileBackendTest extends MediaWikiTestCase { $createOps[] = [ 'op' => 'create', 'dst' => $path, 'content' => mt_rand( 0, 50000 ) ]; $copyOps[] = [ 'op' => 'copy', 'src' => $path, 'dst' => "$path-2" ]; $moveOps[] = [ 'op' => 'move', 'src' => "$path-2", 'dst' => "$path-3" ]; - $purgeOps[] = [ 'op' => 'delete', 'src' => $path ]; - $purgeOps[] = [ 'op' => 'delete', 'src' => "$path-3" ]; + $delOps[] = [ 'op' => 'delete', 'src' => $path ]; + $delOps[] = [ 'op' => 'delete', 'src' => "$path-3" ]; + $delOps[] = [ 'op' => 'delete', 'src' => "$path-gone", 'ignoreMissingSource' => true ]; } - $purgeOps[] = [ 'op' => 'null' ]; + $delOps[] = [ 'op' => 'null' ]; $this->assertGoodStatus( $this->backend->doQuickOperations( $createOps ), @@ -995,7 +996,7 @@ class FileBackendTest extends MediaWikiTestCase { "File {$files[0]} still exists." ); $this->assertGoodStatus( - $this->backend->doQuickOperations( $purgeOps ), + $this->backend->doQuickOperations( $delOps ), "Quick deletion of source files succeeded ($backendName)." ); foreach ( $files as $file ) { $this->assertFalse( $this->backend->fileExists( [ 'src' => $file ] ), -- 2.20.1