Merge pull request #17189 from owncloud/files-straypartfilesonexception

Cleanup part file after upload exception
2015-06-30 11:58:26 +02:00 · 2015-06-30 11:58:26 +02:00 · 2bcd0af177
parent a1bfc26b88 167f57c15e
commit 2bcd0af177
4 changed files with 416 additions and 61 deletions
--- a/lib/private/connector/sabre/file.php
+++ b/lib/private/connector/sabre/file.php
@ -96,7 +96,11 @@ class File extends Node implements IFile {

 		// chunked handling
 		if (isset($_SERVER['HTTP_OC_CHUNKED'])) {
-			return $this->createFileChunked($data);
+			try {
+				return $this->createFileChunked($data);
+			} catch (\Exception $e) {
+				$this->convertToSabreException($e);
+			}
 		}

 		list($partStorage) = $this->fileView->resolvePath($this->path);
@ -125,7 +129,6 @@ class File extends Node implements IFile {
 			$target = $partStorage->fopen($internalPartPath, 'wb');
 			if ($target === false) {
 				\OC_Log::write('webdav', '\OC\Files\Filesystem::fopen() failed', \OC_Log::ERROR);
-				$partStorage->unlink($internalPartPath);
 				// because we have no clue about the cause we can only throw back a 500/Internal Server Error
 				throw new Exception('Could not write file contents');
 			}
@ -138,32 +141,13 @@ class File extends Node implements IFile {
 			if (isset($_SERVER['CONTENT_LENGTH']) && $_SERVER['REQUEST_METHOD'] !== 'LOCK') {
 				$expected = $_SERVER['CONTENT_LENGTH'];
 				if ($count != $expected) {
-					$partStorage->unlink($internalPartPath);
 					throw new BadRequest('expected filesize ' . $expected . ' got ' . $count);
 				}
 			}

-		} catch (NotPermittedException $e) {
-			// a more general case - due to whatever reason the content could not be written
-			throw new Forbidden($e->getMessage());
-		} catch (EntityTooLargeException $e) {
-			// the file is too big to be stored
-			throw new EntityTooLarge($e->getMessage());
-		} catch (InvalidContentException $e) {
-			// the file content is not permitted
-			throw new UnsupportedMediaType($e->getMessage());
-		} catch (InvalidPathException $e) {
-			// the path for the file was not valid
-			// TODO: find proper http status code for this case
-			throw new Forbidden($e->getMessage());
-		} catch (LockNotAcquiredException $e) {
-			// the file is currently being written to by another process
-			throw new FileLocked($e->getMessage(), $e->getCode(), $e);
-		} catch (GenericEncryptionException $e) {
-			// returning 503 will allow retry of the operation at a later point in time
-			throw new ServiceUnavailable("Encryption not ready: " . $e->getMessage());
-		} catch (StorageNotAvailableException $e) {
-			throw new ServiceUnavailable("Failed to write file contents: " . $e->getMessage());
+		} catch (\Exception $e) {
+			$partStorage->unlink($internalPartPath);
+			$this->convertToSabreException($e);
 		}

 		try {
@ -192,6 +176,7 @@ class File extends Node implements IFile {
 			try {
 				$this->fileView->changeLock($this->path, ILockingProvider::LOCK_EXCLUSIVE);
 			} catch (LockedException $e) {
+				$partStorage->unlink($internalPartPath);
 				throw new FileLocked($e->getMessage(), $e->getCode(), $e);
 			}

@ -207,9 +192,9 @@ class File extends Node implements IFile {
 						$partStorage->unlink($internalPartPath);
 						throw new Exception('Could not rename part file to final file');
 					}
-				} catch (\OCP\Files\LockNotAcquiredException $e) {
-					// the file is currently being written to by another process
-					throw new FileLocked($e->getMessage(), $e->getCode(), $e);
+				} catch (\Exception $e) {
+					$partStorage->unlink($internalPartPath);
+					$this->convertToSabreException($e);
 				}
 			}

@ -380,6 +365,9 @@ class File extends Node implements IFile {
 						\OC_Log::write('webdav', '\OC\Files\Filesystem::rename() failed', \OC_Log::ERROR);
 						// only delete if an error occurred and the target file was already created
 						if ($fileExists) {
+							// set to null to avoid double-deletion when handling exception
+							// stray part file
+							$partFile = null;
 							$this->fileView->unlink($targetPath);
 						}
 						throw new Exception('Could not rename part file assembled from chunks');
@ -399,10 +387,11 @@ class File extends Node implements IFile {

 				$info = $this->fileView->getFileInfo($targetPath);
 				return $info->getEtag();
-			} catch (StorageNotAvailableException $e) {
-				throw new ServiceUnavailable("Failed to put file: " . $e->getMessage());
-			} catch (LockedException $e) {
-				throw new FileLocked($e->getMessage(), $e->getCode(), $e);
+			} catch (\Exception $e) {
+				if ($partFile) {
+					$this->fileView->unlink($partFile);
+				}
+				$this->convertToSabreException($e);
 			}
 		}

@ -423,4 +412,47 @@ class File extends Node implements IFile {
 		return !$storage->instanceOfStorage('OCA\Files_Sharing\External\Storage') &&
 		!$storage->instanceOfStorage('OC\Files\Storage\OwnCloud');
 	}
+
+	/**
+	 * Convert the given exception to a SabreException instance
+	 *
+	 * @param \Exception $e
+	 *
+	 * @throws \Sabre\DAV\Exception
+	 */
+	private function convertToSabreException(\Exception $e) {
+		if ($e instanceof \Sabre\DAV\Exception) {
+			throw $e;
+		}
+		if ($e instanceof NotPermittedException) {
+			// a more general case - due to whatever reason the content could not be written
+			throw new Forbidden($e->getMessage(), 0, $e);
+		}
+		if ($e instanceof EntityTooLargeException) {
+			// the file is too big to be stored
+			throw new EntityTooLarge($e->getMessage(), 0, $e);
+		}
+		if ($e instanceof InvalidContentException) {
+			// the file content is not permitted
+			throw new UnsupportedMediaType($e->getMessage(), 0, $e);
+		}
+		if ($e instanceof InvalidPathException) {
+			// the path for the file was not valid
+			// TODO: find proper http status code for this case
+			throw new Forbidden($e->getMessage(), 0, $e);
+		}
+		if ($e instanceof LockedException || $e instanceof LockNotAcquiredException) {
+			// the file is currently being written to by another process
+			throw new FileLocked($e->getMessage(), $e->getCode(), $e);
+		}
+		if ($e instanceof GenericEncryptionException) {
+			// returning 503 will allow retry of the operation at a later point in time
+			throw new ServiceUnavailable('Encryption not ready: ' . $e->getMessage(), 0, $e);
+		}
+		if ($e instanceof StorageNotAvailableException) {
+			throw new ServiceUnavailable('Failed to write file contents: ' . $e->getMessage(), 0, $e);
+		}
+
+		throw new \Sabre\DAV\Exception($e->getMessage(), 0, $e);
+	}
 }
--- a/lib/private/files/view.php
+++ b/lib/private/files/view.php
@ -631,7 +631,12 @@ class View {
 			}

 			$this->lockFile($path1, ILockingProvider::LOCK_SHARED, true);
-			$this->lockFile($path2, ILockingProvider::LOCK_SHARED, true);
+			try {
+				$this->lockFile($path2, ILockingProvider::LOCK_SHARED, true);
+			} catch (LockedException $e) {
+				$this->unlockFile($path1, ILockingProvider::LOCK_SHARED);
+				throw $e;
+			}

 			$run = true;
 			if ($this->shouldEmitHooks() && (Cache\Scanner::isPartialFile($path1) && !Cache\Scanner::isPartialFile($path2))) {
--- a/tests/lib/connector/sabre/file.php
+++ b/tests/lib/connector/sabre/file.php
@ -10,6 +10,7 @@ namespace Test\Connector\Sabre;

 use Test\HookHelper;
 use OC\Files\Filesystem;
+use OCP\Lock\ILockingProvider;

 class File extends \Test\TestCase {

@ -33,6 +34,7 @@ class File extends \Test\TestCase {
 	public function tearDown() {
 		$userManager = \OC::$server->getUserManager();
 		$userManager->get($this->user)->delete();
+		unset($_SERVER['HTTP_OC_CHUNKED']);

 		parent::tearDown();
 	}
@ -44,23 +46,92 @@ class File extends \Test\TestCase {
 		return $stream;
 	}

+
+	public function fopenFailuresProvider() {
+		return [
+			[
+				// return false
+				null,
+				'\Sabre\Dav\Exception',
+				false
+			],
+			[
+				new \OCP\Files\NotPermittedException(),
+				'Sabre\DAV\Exception\Forbidden'
+			],
+			[
+				new \OCP\Files\EntityTooLargeException(),
+				'OC\Connector\Sabre\Exception\EntityTooLarge'
+			],
+			[
+				new \OCP\Files\InvalidContentException(),
+				'OC\Connector\Sabre\Exception\UnsupportedMediaType'
+			],
+			[
+				new \OCP\Files\InvalidPathException(),
+				'Sabre\DAV\Exception\Forbidden'
+			],
+			[
+				new \OCP\Files\LockNotAcquiredException('/test.txt', 1),
+				'OC\Connector\Sabre\Exception\FileLocked'
+			],
+			[
+				new \OCP\Lock\LockedException('/test.txt'),
+				'OC\Connector\Sabre\Exception\FileLocked'
+			],
+			[
+				new \OCP\Encryption\Exceptions\GenericEncryptionException(),
+				'Sabre\DAV\Exception\ServiceUnavailable'
+			],
+			[
+				new \OCP\Files\StorageNotAvailableException(),
+				'Sabre\DAV\Exception\ServiceUnavailable'
+			],
+			[
+				new \Sabre\DAV\Exception('Generic sabre exception'),
+				'Sabre\DAV\Exception',
+				false
+			],
+			[
+				new \Exception('Generic exception'),
+				'Sabre\DAV\Exception'
+			],
+		];
+	}
+
 	/**
-	 * @expectedException \Sabre\DAV\Exception
+	 * @dataProvider fopenFailuresProvider
 	 */
-	public function testSimplePutFails() {
+	public function testSimplePutFails($thrownException, $expectedException, $checkPreviousClass = true) {
 		// setup
-		$storage = $this->getMock('\OC\Files\Storage\Local', ['fopen'], [['datadir' => \OC::$server->getTempManager()->getTemporaryFolder()]]);
+		$storage = $this->getMock(
+			'\OC\Files\Storage\Local',
+			['fopen'],
+			[['datadir' => \OC::$server->getTempManager()->getTemporaryFolder()]]
+		);
+		\OC\Files\Filesystem::mount($storage, [], $this->user . '/');
 		$view = $this->getMock('\OC\Files\View', array('getRelativePath', 'resolvePath'), array());
-		$view->expects($this->any())
+		$view->expects($this->atLeastOnce())
 			->method('resolvePath')
-			->will($this->returnValue(array($storage, '')));
-		$storage->expects($this->once())
-			->method('fopen')
-			->will($this->returnValue(false));
+			->will($this->returnCallback(
+				function($path) use ($storage){
+					return [$storage, $path];
+				}
+			));
+
+		if ($thrownException !== null) {
+			$storage->expects($this->once())
+				->method('fopen')
+				->will($this->throwException($thrownException));
+		} else {
+			$storage->expects($this->once())
+				->method('fopen')
+				->will($this->returnValue(false));
+		}

 		$view->expects($this->any())
 			->method('getRelativePath')
-			->will($this->returnValue('/test.txt'));
+			->will($this->returnArgument(0));

 		$info = new \OC\Files\FileInfo('/test.txt', null, null, array(
 			'permissions' => \OCP\Constants::PERMISSION_ALL
@ -69,9 +140,97 @@ class File extends \Test\TestCase {
 		$file = new \OC\Connector\Sabre\File($view, $info);

 		// action
-		$file->put('test data');
+		$caughtException = null;
+		try {
+			$file->put('test data');
+		} catch (\Exception $e) {
+			$caughtException = $e;
+		}
+
+		$this->assertInstanceOf($expectedException, $caughtException);
+		if ($checkPreviousClass) {
+			$this->assertInstanceOf(get_class($thrownException), $caughtException->getPrevious());
+		}
+
+		$this->assertEmpty($this->listPartFiles($view, ''), 'No stray part files');
 	}

+	/**
+	 * Test putting a file using chunking
+	 *
+	 * @dataProvider fopenFailuresProvider
+	 */
+	public function testChunkedPutFails($thrownException, $expectedException, $checkPreviousClass = false) {
+		// setup
+		$storage = $this->getMock(
+			'\OC\Files\Storage\Local',
+			['fopen'],
+			[['datadir' => \OC::$server->getTempManager()->getTemporaryFolder()]]
+		);
+		\OC\Files\Filesystem::mount($storage, [], $this->user . '/');
+		$view = $this->getMock('\OC\Files\View', ['getRelativePath', 'resolvePath'], []);
+		$view->expects($this->atLeastOnce())
+			->method('resolvePath')
+			->will($this->returnCallback(
+				function($path) use ($storage){
+					return [$storage, $path];
+				}
+			));
+
+		if ($thrownException !== null) {
+			$storage->expects($this->once())
+				->method('fopen')
+				->will($this->throwException($thrownException));
+		} else {
+			$storage->expects($this->once())
+				->method('fopen')
+				->will($this->returnValue(false));
+		}
+
+		$view->expects($this->any())
+			->method('getRelativePath')
+			->will($this->returnArgument(0));
+
+		$_SERVER['HTTP_OC_CHUNKED'] = true;
+
+		$info = new \OC\Files\FileInfo('/test.txt-chunking-12345-2-0', null, null, [
+			'permissions' => \OCP\Constants::PERMISSION_ALL
+		], null);
+		$file = new \OC\Connector\Sabre\File($view, $info);
+
+		// put first chunk
+		$this->assertNull($file->put('test data one'));
+
+		$info = new \OC\Files\FileInfo('/test.txt-chunking-12345-2-1', null, null, [
+			'permissions' => \OCP\Constants::PERMISSION_ALL
+		], null);
+		$file = new \OC\Connector\Sabre\File($view, $info);
+
+		// action
+		$caughtException = null;
+		try {
+			// last chunk
+			$file->put('test data two');
+		} catch (\Exception $e) {
+			$caughtException = $e;
+		}
+
+		$this->assertInstanceOf($expectedException, $caughtException);
+		if ($checkPreviousClass) {
+			$this->assertInstanceOf(get_class($thrownException), $caughtException->getPrevious());
+		}
+
+		$this->assertEmpty($this->listPartFiles($view, ''), 'No stray part files');
+	}
+
+	/**
+	 * Simulate putting a file to the given path.
+	 *
+	 * @param string $path path to put the file into
+	 * @param string $viewRoot root to use for the view
+	 *
+	 * @return result of the PUT operaiton which is usually the etag
+	 */
 	private function doPut($path, $viewRoot = null) {
 		$view = \OC\Files\Filesystem::getView();
 		if (!is_null($viewRoot)) {
@ -90,14 +249,23 @@ class File extends \Test\TestCase {

 		$file = new \OC\Connector\Sabre\File($view, $info);

-		$this->assertNotEmpty($file->put($this->getStream('test data')));
+		return $file->put($this->getStream('test data'));
 	}

 	/**
 	 * Test putting a single file
 	 */
 	public function testPutSingleFile() {
-		$this->doPut('/foo.txt');
+		$this->assertNotEmpty($this->doPut('/foo.txt'));
+	}
+
+	/**
+	 * Test putting a file using chunking
+	 */
+	public function testChunkedPut() {
+		$_SERVER['HTTP_OC_CHUNKED'] = true;
+		$this->assertNull($this->doPut('/test.txt-chunking-12345-2-0'));
+		$this->assertNotEmpty($this->doPut('/test.txt-chunking-12345-2-1'));
 	}

 	/**
@ -106,7 +274,7 @@ class File extends \Test\TestCase {
 	public function testPutSingleFileTriggersHooks() {
 		HookHelper::setUpHooks();

-		$this->doPut('/foo.txt');
+		$this->assertNotEmpty($this->doPut('/foo.txt'));

 		$this->assertCount(4, HookHelper::$hookCalls);
 		$this->assertHookCall(
@ -140,7 +308,7 @@ class File extends \Test\TestCase {

 		HookHelper::setUpHooks();

-		$this->doPut('/foo.txt');
+		$this->assertNotEmpty($this->doPut('/foo.txt'));

 		$this->assertCount(4, HookHelper::$hookCalls);
 		$this->assertHookCall(
@ -177,7 +345,7 @@ class File extends \Test\TestCase {
 		HookHelper::setUpHooks();

 		// happens with public webdav where the view root is the share root
-		$this->doPut('/foo.txt', '/' . $this->user . '/files/noderoot');
+		$this->assertNotEmpty($this->doPut('/foo.txt', '/' . $this->user . '/files/noderoot'));

 		$this->assertCount(4, HookHelper::$hookCalls);
 		$this->assertHookCall(
@ -211,8 +379,6 @@ class File extends \Test\TestCase {

 	/**
 	 * Test put file with cancelled hook
-	 *
-	 * @expectedException \Sabre\DAV\Exception
 	 */
 	public function testPutSingleFileCancelPreHook() {
 		\OCP\Util::connectHook(
@ -222,13 +388,22 @@ class File extends \Test\TestCase {
 			'cancellingCallback'
 		);

-		$this->doPut('/foo.txt');
+		// action
+		$thrown = false;
+		try {
+			$this->doPut('/foo.txt');
+		} catch (\Sabre\DAV\Exception $e) {
+			$thrown = true;
+		}
+
+		$this->assertTrue($thrown);
+		$this->assertEmpty($this->listPartFiles(), 'No stray part files');
 	}

 	/**
-	 * @expectedException \Sabre\DAV\Exception
+	 * Test exception when the uploaded size did not match
 	 */
-	public function testSimplePutFailsOnRename() {
+	public function testSimplePutFailsSizeCheck() {
 		// setup
 		$view = $this->getMock('\OC\Files\View',
 			array('rename', 'getRelativePath', 'filesize'));
@ -238,7 +413,8 @@ class File extends \Test\TestCase {
 			->will($this->returnValue(false));
 		$view->expects($this->any())
 			->method('getRelativePath')
-			->will($this->returnValue('/test.txt'));
+			->will($this->returnArgument(0));
+
 		$view->expects($this->any())
 			->method('filesize')
 			->will($this->returnValue(123456));
@ -253,18 +429,87 @@ class File extends \Test\TestCase {
 		$file = new \OC\Connector\Sabre\File($view, $info);

 		// action
-		$file->put($this->getStream('test data'));
+		$thrown = false;
+		try {
+			$file->put($this->getStream('test data'));
+		} catch (\Sabre\DAV\Exception\BadRequest $e) {
+			$thrown = true;
+		}
+
+		$this->assertTrue($thrown);
+		$this->assertEmpty($this->listPartFiles($view, ''), 'No stray part files');
 	}

 	/**
-	 * @expectedException \OC\Connector\Sabre\Exception\InvalidPath
+	 * Test exception during final rename in simple upload mode
+	 */
+	public function testSimplePutFailsMoveFromStorage() {
+		$view = new \OC\Files\View('/' . $this->user . '/files');
+
+		// simulate situation where the target file is locked
+		$view->lockFile('/test.txt', ILockingProvider::LOCK_EXCLUSIVE);
+
+		$info = new \OC\Files\FileInfo('/' . $this->user . '/files/test.txt', null, null, array(
+			'permissions' => \OCP\Constants::PERMISSION_ALL
+		), null);
+
+		$file = new \OC\Connector\Sabre\File($view, $info);
+
+		// action
+		$thrown = false;
+		try {
+			$file->put($this->getStream('test data'));
+		} catch (\OC\Connector\Sabre\Exception\FileLocked $e) {
+			$thrown = true;
+		}
+
+		$this->assertTrue($thrown);
+		$this->assertEmpty($this->listPartFiles($view, ''), 'No stray part files');
+	}
+
+	/**
+	 * Test exception during final rename in chunk upload mode
+	 */
+	public function testChunkedPutFailsFinalRename() {
+		$view = new \OC\Files\View('/' . $this->user . '/files');
+
+		// simulate situation where the target file is locked
+		$view->lockFile('/test.txt', ILockingProvider::LOCK_EXCLUSIVE);
+
+		$_SERVER['HTTP_OC_CHUNKED'] = true;
+
+		$info = new \OC\Files\FileInfo('/' . $this->user . '/files/test.txt-chunking-12345-2-0', null, null, [
+			'permissions' => \OCP\Constants::PERMISSION_ALL
+		], null);
+		$file = new \OC\Connector\Sabre\File($view, $info);
+		$this->assertNull($file->put('test data one'));
+
+		$info = new \OC\Files\FileInfo('/' . $this->user . '/files/test.txt-chunking-12345-2-1', null, null, [
+			'permissions' => \OCP\Constants::PERMISSION_ALL
+		], null);
+		$file = new \OC\Connector\Sabre\File($view, $info);
+
+		// action
+		$thrown = false;
+		try {
+			$file->put($this->getStream('test data'));
+		} catch (\OC\Connector\Sabre\Exception\FileLocked $e) {
+			$thrown = true;
+		}
+
+		$this->assertTrue($thrown);
+		$this->assertEmpty($this->listPartFiles($view, ''), 'No stray part files');
+	}
+
+	/**
+	 * Test put file with invalid chars
 	 */
 	public function testSimplePutInvalidChars() {
 		// setup
 		$view = $this->getMock('\OC\Files\View', array('getRelativePath'));
 		$view->expects($this->any())
 			->method('getRelativePath')
-			->will($this->returnValue('/*'));
+			->will($this->returnArgument(0));

 		$info = new \OC\Files\FileInfo('/*', null, null, array(
 			'permissions' => \OCP\Constants::PERMISSION_ALL
@ -272,7 +517,15 @@ class File extends \Test\TestCase {
 		$file = new \OC\Connector\Sabre\File($view, $info);

 		// action
-		$file->put($this->getStream('test data'));
+		$thrown = false;
+		try {
+			$file->put($this->getStream('test data'));
+		} catch (\OC\Connector\Sabre\Exception\InvalidPath $e) {
+			$thrown = true;
+		}
+
+		$this->assertTrue($thrown);
+		$this->assertEmpty($this->listPartFiles($view, ''), 'No stray part files');
 	}

 	/**
@ -286,7 +539,7 @@ class File extends \Test\TestCase {

 		$view->expects($this->any())
 			->method('getRelativePath')
-			->will($this->returnValue('/*'));
+			->will($this->returnArgument(0));

 		$info = new \OC\Files\FileInfo('/*', null, null, array(
 			'permissions' => \OCP\Constants::PERMISSION_ALL
@ -296,7 +549,6 @@ class File extends \Test\TestCase {
 	}

 	/**
-	 * @expectedException \Sabre\DAV\Exception\BadRequest
 	 */
 	public function testUploadAbort() {
 		// setup
@ -308,7 +560,7 @@ class File extends \Test\TestCase {
 			->will($this->returnValue(false));
 		$view->expects($this->any())
 			->method('getRelativePath')
-			->will($this->returnValue('/test.txt'));
+			->will($this->returnArgument(0));
 		$view->expects($this->any())
 			->method('filesize')
 			->will($this->returnValue(123456));
@ -323,7 +575,15 @@ class File extends \Test\TestCase {
 		$file = new \OC\Connector\Sabre\File($view, $info);

 		// action
-		$file->put($this->getStream('test data'));
+		$thrown = false;
+		try {
+			$file->put($this->getStream('test data'));
+		} catch (\Sabre\DAV\Exception\BadRequest $e) {
+			$thrown = true;
+		}
+
+		$this->assertTrue($thrown);
+		$this->assertEmpty($this->listPartFiles($view, ''), 'No stray part files');
 	}

 	/**
@ -484,4 +744,29 @@ class File extends \Test\TestCase {
 		);
 	}

+	/**
+	 * Returns part files in the given path
+	 *
+	 * @param \OC\Files\View view which root is the current user's "files" folder
+	 * @param string $path path for which to list part files
+	 *
+	 * @return array list of part files
+	 */
+	private function listPartFiles(\OC\Files\View $userView = null, $path = '') {
+		if ($userView === null) {
+			$userView = \OC\Files\Filesystem::getView();
+		}
+		$files = [];
+		list($storage, $internalPath) = $userView->resolvePath($path);
+		$realPath = $storage->getSourcePath($internalPath);
+		$dh = opendir($realPath);
+		while (($file = readdir($dh)) !== false) {
+			if (substr($file, strlen($file) - 5, 5) === '.part') {
+				$files[] = $file;
+			}
+		}
+		closedir($dh);
+		return $files;
+	}
+
 }
--- a/tests/lib/files/view.php
+++ b/tests/lib/files/view.php
@ -1758,6 +1758,39 @@ class View extends \Test\TestCase {
 		$this->assertNull($this->getFileLockType($view, $targetPath), 'Target file not locked after operation');
 	}

+	/**
+	 * Test rename operation: unlock first path when second path was locked
+	 */
+	public function testLockFileRenameUnlockOnException() {
+		$this->loginAsUser('test');
+
+		$view = new \OC\Files\View('/' . $this->user . '/files/');
+
+		$sourcePath = 'original.txt';
+		$targetPath = 'target.txt';
+		$view->file_put_contents($sourcePath, 'meh');
+
+		// simulate that the target path is already locked
+		$view->lockFile($targetPath, ILockingProvider::LOCK_EXCLUSIVE);
+
+		$this->assertNull($this->getFileLockType($view, $sourcePath), 'Source file not locked before operation');
+		$this->assertEquals(ILockingProvider::LOCK_EXCLUSIVE, $this->getFileLockType($view, $targetPath), 'Target file is locked before operation');
+
+		$thrown = false;
+		try {
+			$view->rename($sourcePath, $targetPath);
+		} catch (\OCP\Lock\LockedException $e) {
+			$thrown = true;
+		}
+
+		$this->assertTrue($thrown, 'LockedException thrown');
+
+		$this->assertNull($this->getFileLockType($view, $sourcePath), 'Source file not locked after operation');
+		$this->assertEquals(ILockingProvider::LOCK_EXCLUSIVE, $this->getFileLockType($view, $targetPath), 'Target file still locked after operation');
+
+		$view->unlockFile($targetPath, ILockingProvider::LOCK_EXCLUSIVE);
+	}
+
 	public function lockFileRenameOrCopyCrossStorageDataProvider() {
 		return [
 			['rename', 'moveFromStorage', ILockingProvider::LOCK_EXCLUSIVE],