Merge pull request #19247 from owncloud/fix_locking_copy_operation

locking: handle exceptions correctly during copy operation

2 files changed, 98 insertions, 42 deletions

diff --git a/lib/private/files/view.php b/lib/private/files/view.php
index 9afa9d40b20..c04ca2ef461 100644
--- a/lib/private/files/view.php
+++ b/lib/private/files/view.php
@@ -759,59 +759,72 @@ class View {
 
 			$this->lockFile($path2, ILockingProvider::LOCK_SHARED);
 			$this->lockFile($path1, ILockingProvider::LOCK_SHARED);
+			$lockTypePath1 = ILockingProvider::LOCK_SHARED;
+			$lockTypePath2 = ILockingProvider::LOCK_SHARED;
 
-			$exists = $this->file_exists($path2);
-			if ($this->shouldEmitHooks()) {
-				\OC_Hook::emit(
-					Filesystem::CLASSNAME,
-					Filesystem::signal_copy,
-					array(
-						Filesystem::signal_param_oldpath => $this->getHookPath($path1),
-						Filesystem::signal_param_newpath => $this->getHookPath($path2),
-						Filesystem::signal_param_run => &$run
-					)
-				);
-				$this->emit_file_hooks_pre($exists, $path2, $run);
-			}
-			if ($run) {
-				$mount1 = $this->getMount($path1);
-				$mount2 = $this->getMount($path2);
-				$storage1 = $mount1->getStorage();
-				$internalPath1 = $mount1->getInternalPath($absolutePath1);
-				$storage2 = $mount2->getStorage();
-				$internalPath2 = $mount2->getInternalPath($absolutePath2);
-
-				$this->changeLock($path2, ILockingProvider::LOCK_EXCLUSIVE);
-
-				if ($mount1->getMountPoint() == $mount2->getMountPoint()) {
-					if ($storage1) {
-						$result = $storage1->copy($internalPath1, $internalPath2);
-					} else {
-						$result = false;
-					}
-				} else {
-					$result = $storage2->copyFromStorage($storage1, $internalPath1, $internalPath2);
-				}
-
-				$this->updater->update($path2);
-
-				$this->changeLock($path2, ILockingProvider::LOCK_SHARED);
+			try {
 
-				if ($this->shouldEmitHooks() && $result !== false) {
+				$exists = $this->file_exists($path2);
+				if ($this->shouldEmitHooks()) {
 					\OC_Hook::emit(
 						Filesystem::CLASSNAME,
-						Filesystem::signal_post_copy,
+						Filesystem::signal_copy,
 						array(
 							Filesystem::signal_param_oldpath => $this->getHookPath($path1),
-							Filesystem::signal_param_newpath => $this->getHookPath($path2)
+							Filesystem::signal_param_newpath => $this->getHookPath($path2),
+							Filesystem::signal_param_run => &$run
 						)
 					);
-					$this->emit_file_hooks_post($exists, $path2);
+					$this->emit_file_hooks_pre($exists, $path2, $run);
 				}
+				if ($run) {
+					$mount1 = $this->getMount($path1);
+					$mount2 = $this->getMount($path2);
+					$storage1 = $mount1->getStorage();
+					$internalPath1 = $mount1->getInternalPath($absolutePath1);
+					$storage2 = $mount2->getStorage();
+					$internalPath2 = $mount2->getInternalPath($absolutePath2);
+
+					$this->changeLock($path2, ILockingProvider::LOCK_EXCLUSIVE);
+					$lockTypePath2 = ILockingProvider::LOCK_EXCLUSIVE;
+
+					if ($mount1->getMountPoint() == $mount2->getMountPoint()) {
+						if ($storage1) {
+							$result = $storage1->copy($internalPath1, $internalPath2);
+						} else {
+							$result = false;
+						}
+					} else {
+						$result = $storage2->copyFromStorage($storage1, $internalPath1, $internalPath2);
+					}
 
-				$this->unlockFile($path2, ILockingProvider::LOCK_SHARED);
-				$this->unlockFile($path1, ILockingProvider::LOCK_SHARED);
+					$this->updater->update($path2);
+
+					$this->changeLock($path2, ILockingProvider::LOCK_SHARED);
+					$lockTypePath2 = ILockingProvider::LOCK_SHARED;
+
+					if ($this->shouldEmitHooks() && $result !== false) {
+						\OC_Hook::emit(
+							Filesystem::CLASSNAME,
+							Filesystem::signal_post_copy,
+							array(
+								Filesystem::signal_param_oldpath => $this->getHookPath($path1),
+								Filesystem::signal_param_newpath => $this->getHookPath($path2)
+							)
+						);
+						$this->emit_file_hooks_post($exists, $path2);
+					}
+
+				}
+			} catch (\Exception $e) {
+				$this->unlockFile($path2, $lockTypePath2);
+				$this->unlockFile($path1, $lockTypePath1);
+				throw $e;
 			}
+
+			$this->unlockFile($path2, $lockTypePath2);
+			$this->unlockFile($path1, $lockTypePath1);
+
 		}
 		return $result;
 	}
diff --git a/tests/lib/files/view.php b/tests/lib/files/view.php
index abb1696ae70..83f53833855 100644
--- a/tests/lib/files/view.php
+++ b/tests/lib/files/view.php
@@ -1818,6 +1818,49 @@ class View extends \Test\TestCase {
 	}
 
 	/**
+	 * simulate a failed copy operation.
+	 * We expect that we catch the exception, free the lock and re-throw it.
+	 *
+	 * @expectedException \Exception
+	 */
+	public function testLockFileCopyException() {
+		$view = new \OC\Files\View('/' . $this->user . '/files/');
+
+		$storage = $this->getMockBuilder('\OC\Files\Storage\Temporary')
+			->setMethods(['copy'])
+			->getMock();
+
+		$sourcePath = 'original.txt';
+		$targetPath = 'target.txt';
+
+		\OC\Files\Filesystem::mount($storage, array(), $this->user . '/');
+		$storage->mkdir('files');
+		$view->file_put_contents($sourcePath, 'meh');
+
+		$storage->expects($this->once())
+			->method('copy')
+			->will($this->returnCallback(
+				function() {
+					throw new \Exception();
+				}
+			));
+
+		$this->connectMockHooks('copy', $view, $sourcePath, $lockTypeSourcePre, $lockTypeSourcePost);
+		$this->connectMockHooks('copy', $view, $targetPath, $lockTypeTargetPre, $lockTypeTargetPost);
+
+		$this->assertNull($this->getFileLockType($view, $sourcePath), 'Source file not locked before operation');
+		$this->assertNull($this->getFileLockType($view, $targetPath), 'Target file not locked before operation');
+
+		try {
+			$view->copy($sourcePath, $targetPath);
+		} catch (\Exception $e) {
+			$this->assertNull($this->getFileLockType($view, $sourcePath), 'Source file not locked after operation');
+			$this->assertNull($this->getFileLockType($view, $targetPath), 'Target file not locked after operation');
+			throw $e;
+		}
+	}
+
+	/**
 	 * Test rename operation: unlock first path when second path was locked
 	 */
 	public function testLockFileRenameUnlockOnException() {