Skip to content

Commit

Permalink
Merge pull request #17104 from owncloud/chunked-upload-locking
Browse files Browse the repository at this point in the history 
locking for chunked dav upload
  • Loading branch information
@icewind1991
icewind1991 committed Oct 27, 2015
2 parents 4d87276 + 021ed8b commit c309193
Show file tree
Hide file tree
Showing 4 changed files with 104 additions and 13 deletions.
21 changes: 13 additions & 8 deletions apps/dav/lib/connector/sabre/file.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,7 @@ private function createFileChunked($data) {
if (empty($info)) {
throw new NotImplemented('Invalid chunk name');
}

$chunk_handler = new \OC_FileChunking($info);
$bytesWritten = $chunk_handler->store($info['index'], $data);

Expand Down Expand Up @@ -376,24 +377,25 @@ private function createFileChunked($data) {
$exists = $this->fileView->file_exists($targetPath);

try {
$this->emitPreHooks($exists, $targetPath);
$this->fileView->lockFile($targetPath, ILockingProvider::LOCK_SHARED);

$this->changeLock(ILockingProvider::LOCK_EXCLUSIVE);
$this->emitPreHooks($exists, $targetPath);
$this->fileView->changeLock($targetPath, ILockingProvider::LOCK_EXCLUSIVE);
/** @var \OC\Files\Storage\Storage $targetStorage */
list($targetStorage, $targetInternalPath) = $this->fileView->resolvePath($targetPath);

if ($needsPartFile) {
// we first assembly the target file as a part file
$partFile = $path . '/' . $info['name'] . '.ocTransferId' . $info['transferid'] . '.part';


/** @var \OC\Files\Storage\Storage $targetStorage */
list($partStorage, $partInternalPath) = $this->fileView->resolvePath($partFile);


$chunk_handler->file_assemble($partStorage, $partInternalPath, $this->fileView->getAbsolutePath($targetPath));

// here is the final atomic rename
$renameOkay = $targetStorage->moveFromStorage($partStorage, $partInternalPath, $targetInternalPath);

$fileExists = $this->fileView->file_exists($targetPath);
$fileExists = $targetStorage->file_exists($targetInternalPath);
if ($renameOkay === false || $fileExists === false) {
\OCP\Util::writeLog('webdav', '\OC\Files\Filesystem::rename() failed', \OCP\Util::ERROR);
// only delete if an error occurred and the target file was already created
Expand All @@ -403,7 +405,7 @@ private function createFileChunked($data) {
$partFile = null;
$targetStorage->unlink($targetInternalPath);
}
$this->changeLock(ILockingProvider::LOCK_SHARED);
$this->fileView->changeLock($targetPath, ILockingProvider::LOCK_SHARED);
throw new Exception('Could not rename part file assembled from chunks');
}
} else {
Expand All @@ -419,14 +421,17 @@ private function createFileChunked($data) {
}
}

$this->changeLock(ILockingProvider::LOCK_SHARED);
$this->fileView->changeLock($targetPath, ILockingProvider::LOCK_SHARED);

// since we skipped the view we need to scan and emit the hooks ourselves
$this->fileView->getUpdater()->update($targetPath);

$this->emitPostHooks($exists, $targetPath);

$info = $this->fileView->getFileInfo($targetPath);

$this->fileView->unlockFile($targetPath, ILockingProvider::LOCK_SHARED);

return $info->getEtag();
} catch (\Exception $e) {
if ($partFile !== null) {
Expand Down
4 changes: 2 additions & 2 deletions apps/dav/lib/connector/sabre/lockplugin.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ public function initialize(\Sabre\DAV\Server $server) {
public function getLock(RequestInterface $request) {
// we cant listen on 'beforeMethod:PUT' due to order of operations with setting up the tree
// so instead we limit ourselves to the PUT method manually
if ($request->getMethod() !== 'PUT') {
if ($request->getMethod() !== 'PUT' || isset($_SERVER['HTTP_OC_CHUNKED'])) {
return;
}
try {
Expand All @@ -80,7 +80,7 @@ public function getLock(RequestInterface $request) {
}

public function releaseLock(RequestInterface $request) {
if ($request->getMethod() !== 'PUT') {
if ($request->getMethod() !== 'PUT' || isset($_SERVER['HTTP_OC_CHUNKED'])) {
return;
}
try {
Expand Down
10 changes: 8 additions & 2 deletions apps/dav/tests/unit/connector/sabre/file.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,9 @@ function ($path) use ($storage) {
$file = new \OCA\DAV\Connector\Sabre\File($view, $info);

// put first chunk
$file->acquireLock(ILockingProvider::LOCK_SHARED);
$this->assertNull($file->put('test data one'));
$file->releaseLock(ILockingProvider::LOCK_SHARED);

$info = new \OC\Files\FileInfo('/test.txt-chunking-12345-2-1', null, null, [
'permissions' => \OCP\Constants::PERMISSION_ALL
Expand Down Expand Up @@ -443,12 +445,12 @@ public function testSimplePutFailsSizeCheck() {
$thrown = false;
try {
// beforeMethod locks
$view->lockFile('/test.txt', ILockingProvider::LOCK_SHARED);
$file->acquireLock(ILockingProvider::LOCK_SHARED);

$file->put($this->getStream('test data'));

// afterMethod unlocks
$view->unlockFile('/test.txt', ILockingProvider::LOCK_SHARED);
$file->releaseLock(ILockingProvider::LOCK_SHARED);
} catch (\Sabre\DAV\Exception\BadRequest $e) {
$thrown = true;
}
Expand Down Expand Up @@ -505,7 +507,9 @@ public function testChunkedPutFailsFinalRename() {
'permissions' => \OCP\Constants::PERMISSION_ALL
], null);
$file = new \OCA\DAV\Connector\Sabre\File($view, $info);
$file->acquireLock(ILockingProvider::LOCK_SHARED);
$this->assertNull($file->put('test data one'));
$file->releaseLock(ILockingProvider::LOCK_SHARED);

$info = new \OC\Files\FileInfo('/' . $this->user . '/files/test.txt-chunking-12345-2-1', null, null, [
'permissions' => \OCP\Constants::PERMISSION_ALL
Expand All @@ -515,7 +519,9 @@ public function testChunkedPutFailsFinalRename() {
// action
$thrown = false;
try {
$file->acquireLock(ILockingProvider::LOCK_SHARED);
$file->put($this->getStream('test data'));
$file->releaseLock(ILockingProvider::LOCK_SHARED);
} catch (\OCA\DAV\Connector\Sabre\Exception\FileLocked $e) {
$thrown = true;
}
Expand Down
82 changes: 81 additions & 1 deletion apps/dav/tests/unit/connector/sabre/requesttest/uploadtest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@

namespace OCA\DAV\Tests\Unit\Connector\Sabre\RequestTest;

use OC\AppFramework\Http;
use OC\Connector\Sabre\Exception\FileLocked;
use OCP\AppFramework\Http;
use OCP\Lock\ILockingProvider;

class UploadTest extends RequestTest {
public function testBasicUpload() {
Expand Down Expand Up @@ -43,6 +45,34 @@ public function testUploadOverWrite() {
$this->assertEquals(3, $info->getSize());
}

/**
* @expectedException \OCA\DAV\Connector\Sabre\Exception\FileLocked
*/
public function testUploadOverWriteReadLocked() {
$user = $this->getUniqueID();
$view = $this->setupUser($user, 'pass');

$view->file_put_contents('foo.txt', 'bar');

$view->lockFile('/foo.txt', ILockingProvider::LOCK_SHARED);

$this->request($view, $user, 'pass', 'PUT', '/foo.txt', 'asd');
}

/**
* @expectedException \OCA\DAV\Connector\Sabre\Exception\FileLocked
*/
public function testUploadOverWriteWriteLocked() {
$user = $this->getUniqueID();
$view = $this->setupUser($user, 'pass');

$view->file_put_contents('foo.txt', 'bar');

$view->lockFile('/foo.txt', ILockingProvider::LOCK_EXCLUSIVE);

$this->request($view, $user, 'pass', 'PUT', '/foo.txt', 'asd');
}

public function testChunkedUpload() {
$user = $this->getUniqueID();
$view = $this->setupUser($user, 'pass');
Expand Down Expand Up @@ -107,4 +137,54 @@ public function testChunkedUploadOutOfOrder() {
$this->assertInstanceOf('\OC\Files\FileInfo', $info);
$this->assertEquals(6, $info->getSize());
}

/**
* @expectedException \OCA\DAV\Connector\Sabre\Exception\FileLocked
*/
public function testChunkedUploadOutOfOrderReadLocked() {
$user = $this->getUniqueID();
$view = $this->setupUser($user, 'pass');

$this->assertFalse($view->file_exists('foo.txt'));

$view->lockFile('/foo.txt', ILockingProvider::LOCK_SHARED);

try {
$response = $this->request($view, $user, 'pass', 'PUT', '/foo.txt-chunking-123-2-1', 'bar', ['OC-Chunked' => '1']);
} catch (\OCA\DAV\Connector\Sabre\Exception\FileLocked $e) {
$this->fail('Didn\'t expect locked error for the first chunk on read lock');
return;
}

$this->assertEquals(Http::STATUS_CREATED, $response->getStatus());
$this->assertFalse($view->file_exists('foo.txt'));

// last chunk should trigger the locked error since it tries to assemble
$this->request($view, $user, 'pass', 'PUT', '/foo.txt-chunking-123-2-0', 'asd', ['OC-Chunked' => '1']);
}

/**
* @expectedException \OCA\DAV\Connector\Sabre\Exception\FileLocked
*/
public function testChunkedUploadOutOfOrderWriteLocked() {
$user = $this->getUniqueID();
$view = $this->setupUser($user, 'pass');

$this->assertFalse($view->file_exists('foo.txt'));

$view->lockFile('/foo.txt', ILockingProvider::LOCK_EXCLUSIVE);

try {
$response = $this->request($view, $user, 'pass', 'PUT', '/foo.txt-chunking-123-2-1', 'bar', ['OC-Chunked' => '1']);
} catch (\OCA\DAV\Connector\Sabre\Exception\FileLocked $e) {
$this->fail('Didn\'t expect locked error for the first chunk on write lock'); // maybe forbid this in the future for write locks only?
return;
}

$this->assertEquals(Http::STATUS_CREATED, $response->getStatus());
$this->assertFalse($view->file_exists('foo.txt'));

// last chunk should trigger the locked error since it tries to assemble
$this->request($view, $user, 'pass', 'PUT', '/foo.txt-chunking-123-2-0', 'asd', ['OC-Chunked' => '1']);
}
}

0 comments on commit c309193

Please sign in to comment.