Mirrors
/
nextcloud-server
mirror of https://github.com/nextcloud/server
3
0
Fork 0
Code Releases Activity
Browse Source

add locking to resolve concurent move to trashbin conflicts 

uses a lock to prevent two requests from moving a file to the trashbin concurrently
(causing sql duplicate key errors)

Signed-off-by: Robin Appelman <robin@icewind.nl>
pull/20841/head
Robin Appelman 6 years ago
committed by Roeland Jago Douma
parent
06732cf2af
commit
614acc9419
No known key found for this signature in database GPG Key ID: F941078878347C0C
2 changed files with 81 additions and 21 deletions
Whitespace
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Split View
Diff Options
Show Stats Download Patch File Download Diff File
  1. 52
      apps/files_trashbin/lib/Trashbin.php
  2. 50
      apps/files_trashbin/tests/StorageTest.php

52
apps/files_trashbin/lib/Trashbin.php
View File

@ -47,10 +47,13 @@ use OC\Files\Filesystem;
use OC\Files\View;
use OCA\Files_Trashbin\AppInfo\Application;
use OCA\Files_Trashbin\Command\Expire;
use OCP\AppFramework\Utility\ITimeFactory;
use OCP\Files\File;
use OCP\Files\Folder;
use OCP\Files\NotFoundException;
use OCP\Files\NotPermittedException;
use OCP\Lock\ILockingProvider;
use OCP\Lock\LockedException;
use OCP\User;
class Trashbin {
@ -220,8 +223,8 @@ class Trashbin {
public static function move2trash($file_path, $ownerOnly = false) {
// get the user for which the filesystem is setup
$root = Filesystem::getRoot();
list(, $user) = explode('/', $root);
list($owner, $ownerPath) = self::getUidAndFilename($file_path);
[, $user] = explode('/', $root);
[$owner, $ownerPath] = self::getUidAndFilename($file_path);
// if no owner found (ex: ext storage + share link), will use the current user's trashbin then
if (is_null($owner)) {
@ -245,15 +248,36 @@ class Trashbin {
$filename = $path_parts['basename'];
$location = $path_parts['dirname'];
$timestamp = time();
/** @var ITimeFactory $timeFactory */
$timeFactory = \OC::$server->query(ITimeFactory::class);
$timestamp = $timeFactory->getTime();
$lockingProvider = \OC::$server->getLockingProvider();
// disable proxy to prevent recursive calls
$trashPath = '/files_trashbin/files/' . $filename . '.d' . $timestamp;
$gotLock = false;
while (!$gotLock) {
try {
/** @var \OC\Files\Storage\Storage $trashStorage */
[$trashStorage, $trashInternalPath] = $ownerView->resolvePath($trashPath);
$trashStorage->acquireLock($trashInternalPath, ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider);
$gotLock = true;
} catch (LockedException $e) {
// a file with the same name is being deleted concurrently
// nudge the timestamp a bit to resolve the conflict
$timestamp = $timestamp + 1;
$trashPath = '/files_trashbin/files/' . $filename . '.d' . $timestamp;
}
}
/** @var \OC\Files\Storage\Storage $trashStorage */
list($trashStorage, $trashInternalPath) = $ownerView->resolvePath($trashPath);
/** @var \OC\Files\Storage\Storage $sourceStorage */
list($sourceStorage, $sourceInternalPath) = $ownerView->resolvePath('/files/' . $ownerPath);
[$sourceStorage, $sourceInternalPath] = $ownerView->resolvePath('/files/' . $ownerPath);
try {
$moveSuccessful = true;
if ($trashStorage->file_exists($trashInternalPath)) {
@ -296,6 +320,8 @@ class Trashbin {
}
}
$trashStorage->releaseLock($trashInternalPath, ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider);
self::scheduleExpire($user);
// if owner !== user we also need to update the owners trash size
@ -345,9 +371,9 @@ class Trashbin {
*/
private static function move(View $view, $source, $target) {
/** @var \OC\Files\Storage\Storage $sourceStorage */
list($sourceStorage, $sourceInternalPath) = $view->resolvePath($source);
[$sourceStorage, $sourceInternalPath] = $view->resolvePath($source);
/** @var \OC\Files\Storage\Storage $targetStorage */
list($targetStorage, $targetInternalPath) = $view->resolvePath($target);
[$targetStorage, $targetInternalPath] = $view->resolvePath($target);
/** @var \OC\Files\Storage\Storage $ownerTrashStorage */
$result = $targetStorage->moveFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
@ -367,9 +393,9 @@ class Trashbin {
*/
private static function copy(View $view, $source, $target) {
/** @var \OC\Files\Storage\Storage $sourceStorage */
list($sourceStorage, $sourceInternalPath) = $view->resolvePath($source);
[$sourceStorage, $sourceInternalPath] = $view->resolvePath($source);
/** @var \OC\Files\Storage\Storage $targetStorage */
list($targetStorage, $targetInternalPath) = $view->resolvePath($target);
[$targetStorage, $targetInternalPath] = $view->resolvePath($target);
/** @var \OC\Files\Storage\Storage $ownerTrashStorage */
$result = $targetStorage->copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
@ -465,7 +491,7 @@ class Trashbin {
$target = Filesystem::normalizePath('/' . $location . '/' . $uniqueFilename);
list($owner, $ownerPath) = self::getUidAndFilename($target);
[$owner, $ownerPath] = self::getUidAndFilename($target);
// file has been deleted in between
if (empty($ownerPath)) {
@ -731,7 +757,7 @@ class Trashbin {
$dirContent = Helper::getTrashFiles('/', $user, 'mtime');
// delete all files older then $retention_obligation
list($delSize, $count) = self::deleteExpiredFiles($dirContent, $user);
[$delSize, $count] = self::deleteExpiredFiles($dirContent, $user);
$availableSpace += $delSize;
@ -868,7 +894,7 @@ class Trashbin {
//force rescan of versions, local storage may not have updated the cache
if (!self::$scannedVersions) {
/** @var \OC\Files\Storage\Storage $storage */
list($storage,) = $view->resolvePath('/');
[$storage,] = $view->resolvePath('/');
$storage->getScanner()->scan('files_trashbin/versions');
self::$scannedVersions = true;
}

50
apps/files_trashbin/tests/StorageTest.php
View File

@ -37,12 +37,14 @@ use OC\Files\Storage\Temporary;
use OCA\Files_Trashbin\Events\MoveToTrashEvent;
use OCA\Files_Trashbin\Storage;
use OCA\Files_Trashbin\Trash\ITrashManager;
use OCP\AppFramework\Utility\ITimeFactory;
use OCP\Files\Cache\ICache;
use OCP\Files\Folder;
use OCP\Files\IRootFolder;
use OCP\Files\Node;
use OCP\ILogger;
use OCP\IUserManager;
use OCP\Lock\ILockingProvider;
use Symfony\Component\EventDispatcher\EventDispatcherInterface;
/**
@ -107,7 +109,7 @@ class StorageTest extends \Test\TestCase {
public function testSingleStorageDeleteFile() {
$this->assertTrue($this->userView->file_exists('test.txt'));
$this->userView->unlink('test.txt');
list($storage,) = $this->userView->resolvePath('test.txt');
[$storage,] = $this->userView->resolvePath('test.txt');
$storage->getScanner()->scan(''); // make sure we check the storage
$this->assertFalse($this->userView->getFileInfo('test.txt'));
@ -124,7 +126,7 @@ class StorageTest extends \Test\TestCase {
public function testSingleStorageDeleteFolder() {
$this->assertTrue($this->userView->file_exists('folder/inside.txt'));
$this->userView->rmdir('folder');
list($storage,) = $this->userView->resolvePath('folder/inside.txt');
[$storage,] = $this->userView->resolvePath('folder/inside.txt');
$storage->getScanner()->scan(''); // make sure we check the storage
$this->assertFalse($this->userView->getFileInfo('folder'));
@ -213,7 +215,7 @@ class StorageTest extends \Test\TestCase {
$this->userView->unlink('test.txt');
// rescan trash storage
list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin');
[$rootStorage,] = $this->rootView->resolvePath($this->user . '/files_trashbin');
$rootStorage->getScanner()->scan('');
// check if versions are in trashbin
@ -242,7 +244,7 @@ class StorageTest extends \Test\TestCase {
$this->userView->rmdir('folder');
// rescan trash storage
list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin');
[$rootStorage,] = $this->rootView->resolvePath($this->user . '/files_trashbin');
$rootStorage->getScanner()->scan('');
// check if versions are in trashbin
@ -296,7 +298,7 @@ class StorageTest extends \Test\TestCase {
$recipientView->unlink('share/test.txt');
// rescan trash storage for both users
list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin');
[$rootStorage,] = $this->rootView->resolvePath($this->user . '/files_trashbin');
$rootStorage->getScanner()->scan('');
// check if versions are in trashbin for both users
@ -350,7 +352,7 @@ class StorageTest extends \Test\TestCase {
$recipientView->rmdir('share/folder');
// rescan trash storage
list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin');
[$rootStorage,] = $this->rootView->resolvePath($this->user . '/files_trashbin');
$rootStorage->getScanner()->scan('');
// check if versions are in trashbin for owner
@ -407,7 +409,7 @@ class StorageTest extends \Test\TestCase {
$this->assertTrue($this->userView->file_exists('substorage/test.txt'));
// rescan trash storage
list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin');
[$rootStorage,] = $this->rootView->resolvePath($this->user . '/files_trashbin');
$rootStorage->getScanner()->scan('');
// versions were moved too
@ -448,7 +450,7 @@ class StorageTest extends \Test\TestCase {
$this->assertTrue($this->userView->file_exists('substorage/folder/inside.txt'));
// rescan trash storage
list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin');
[$rootStorage,] = $this->rootView->resolvePath($this->user . '/files_trashbin');
$rootStorage->getScanner()->scan('');
// versions were moved too
@ -606,4 +608,36 @@ class StorageTest extends \Test\TestCase {
$this->addToAssertionCount(1);
}
}
public function testTrashbinCollision() {
$this->userView->file_put_contents('test.txt', 'foo');
$this->userView->file_put_contents('folder/test.txt', 'bar');
$timeFactory = $this->createMock(ITimeFactory::class);
$timeFactory->method('getTime')
->willReturn(1000);
$lockingProvider = \OC::$server->getLockingProvider();
$this->overwriteService(ITimeFactory::class, $timeFactory);
$this->userView->unlink('test.txt');
$this->assertTrue($this->rootView->file_exists('/' . $this->user . '/files_trashbin/files/test.txt.d1000'));
/** @var \OC\Files\Storage\Storage $trashStorage */
[$trashStorage, $trashInternalPath] = $this->rootView->resolvePath('/' . $this->user . '/files_trashbin/files/test.txt.d1000');
/// simulate a concurrent delete
$trashStorage->acquireLock($trashInternalPath, ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider);
$this->userView->unlink('folder/test.txt');
$trashStorage->releaseLock($trashInternalPath, ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider);
$this->assertTrue($this->rootView->file_exists($this->user . '/files_trashbin/files/test.txt.d1001'));
$this->assertEquals('foo', $this->rootView->file_get_contents($this->user . '/files_trashbin/files/test.txt.d1000'));
$this->assertEquals('bar', $this->rootView->file_get_contents($this->user . '/files_trashbin/files/test.txt.d1001'));
}
}
Write Preview
Loading…
Cancel
Save