Merge pull request #17259 from owncloud/chunk-cleanupgracefulonlock

Do not try clearing locked files in cache folder

3 files changed, 107 insertions, 10 deletions

diff --git a/lib/base.php b/lib/base.php
index 829c2bcb866..8812d5698f1 100644
--- a/lib/base.php
+++ b/lib/base.php
@@ -730,8 +730,14 @@ class OC {
 			// NOTE: This will be replaced to use OCP
 			$userSession = self::$server->getUserSession();
 			$userSession->listen('\OC\User', 'postLogin', function () {
-				$cache = new \OC\Cache\File();
-				$cache->gc();
+				try {
+					$cache = new \OC\Cache\File();
+					$cache->gc();
+				} catch (\Exception $e) {
+					// a GC exception should not prevent users from using OC,
+					// so log the exception
+					\OC::$server->getLogger()->warning('Exception when running cache gc: ' . $e->getMessage(), array('app' => 'core'));
+				}
 			});
 		}
 	}
diff --git a/lib/private/cache/file.php b/lib/private/cache/file.php
index 32c00125764..69008c7fab5 100644
--- a/lib/private/cache/file.php
+++ b/lib/private/cache/file.php
@@ -177,9 +177,16 @@ class File implements ICache {
 			}
 			while (($file = readdir($dh)) !== false) {
 				if ($file != '.' and $file != '..') {
-					$mtime = $storage->filemtime('/' . $file);
-					if ($mtime < $now) {
-						$storage->unlink('/' . $file);
+					try {
+						$mtime = $storage->filemtime('/' . $file);
+						if ($mtime < $now) {
+							$storage->unlink('/' . $file);
+						}
+					} catch (\OCP\Lock\LockedException $e) {
+						// ignore locked chunks
+						\OC::$server->getLogger()->debug('Could not cleanup locked chunk "' . $file . '"', array('app' => 'core'));
+					} catch (\OCP\Files\LockNotAcquiredException $e) {
+						\OC::$server->getLogger()->debug('Could not cleanup locked chunk "' . $file . '"', array('app' => 'core'));
 					}
 				}
 			}
diff --git a/tests/lib/cache/file.php b/tests/lib/cache/file.php
index e31f84ee6f1..94001291d86 100644
--- a/tests/lib/cache/file.php
+++ b/tests/lib/cache/file.php
@@ -23,12 +23,22 @@
 namespace Test\Cache;
 
 class FileCache extends \Test_Cache {
-	/** @var string */
+	/**
+	 * @var string
+	 * */
 	private $user;
-	/** @var string */
+	/**
+	 * @var string
+	 * */
 	private $datadir;
-	/** @var \OC\Files\Storage\Storage */
+	/**
+	 * @var \OC\Files\Storage\Storage
+	 * */
 	private $storage;
+	/**
+	 * @var \OC\Files\View
+	 * */
+	private $rootView;
 
 	function skip() {
 		//$this->skipUnless(OC_User::isLoggedIn());
@@ -59,13 +69,18 @@ class FileCache extends \Test_Cache {
 		\OC_User::setUserId('test');
 
 		//set up the users dir
-		$rootView = new \OC\Files\View('');
-		$rootView->mkdir('/test');
+		$this->rootView = new \OC\Files\View('');
+		$this->rootView->mkdir('/test');
 
 		$this->instance=new \OC\Cache\File();
+
+		// forces creation of cache folder for subsequent tests
+		$this->instance->set('hack', 'hack');
 	}
 
 	protected function tearDown() {
+		$this->instance->remove('hack', 'hack');
+
 		\OC_User::setUserId($this->user);
 		\OC_Config::setValue('cachedirectory', $this->datadir);
 
@@ -75,4 +90,73 @@ class FileCache extends \Test_Cache {
 
 		parent::tearDown();
 	}
+
+	private function setupMockStorage() {
+		$mockStorage = $this->getMock(
+			'\OC\Files\Storage\Local',
+			['filemtime', 'unlink'],
+			[['datadir' => \OC::$server->getTempManager()->getTemporaryFolder()]]
+		);
+
+		\OC\Files\Filesystem::mount($mockStorage, array(), '/test/cache');
+
+		return $mockStorage;
+	}
+
+	public function testGarbageCollectOldKeys() {
+		$mockStorage = $this->setupMockStorage();
+
+		$mockStorage->expects($this->atLeastOnce())
+			->method('filemtime')
+			->will($this->returnValue(100));
+		$mockStorage->expects($this->once())
+			->method('unlink')
+			->with('key1')
+			->will($this->returnValue(true));
+
+		$this->instance->set('key1', 'value1');
+		$this->instance->gc();
+	}
+
+	public function testGarbageCollectLeaveRecentKeys() {
+		$mockStorage = $this->setupMockStorage();
+
+		$mockStorage->expects($this->atLeastOnce())
+			->method('filemtime')
+			->will($this->returnValue(time() + 3600));
+		$mockStorage->expects($this->never())
+			->method('unlink')
+			->with('key1');
+		$this->instance->set('key1', 'value1');
+		$this->instance->gc();
+	}
+
+	public function lockExceptionProvider() {
+		return [
+			[new \OCP\Lock\LockedException('key1')],
+			[new \OCP\Files\LockNotAcquiredException('key1', 1)],
+		];
+	}
+
+	/**
+	 * @dataProvider lockExceptionProvider
+	 */
+	public function testGarbageCollectIgnoreLockedKeys($testException) {
+		$mockStorage = $this->setupMockStorage();
+
+		$mockStorage->expects($this->atLeastOnce())
+			->method('filemtime')
+			->will($this->returnValue(100));
+		$mockStorage->expects($this->atLeastOnce())
+			->method('unlink')
+			->will($this->onConsecutiveCalls(
+				$this->throwException($testException),
+				$this->returnValue(true)
+			));
+
+		$this->instance->set('key1', 'value1');
+		$this->instance->set('key2', 'value2');
+
+		$this->instance->gc();
+	}
 }