Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

locking for chunked dav upload #17104

Merged
merged 6 commits into from
Oct 27, 2015
Merged

locking for chunked dav upload #17104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
23eaf27
locking for chunked dav upload
icewind1991 Jun 23, 2015
21d0267
Add tests for uploading to locked files
icewind1991 Sep 14, 2015
ddc8749
Adjust for wide locking
icewind1991 Sep 15, 2015
760335c
fix locking in tests
icewind1991 Oct 5, 2015
283798a
remove locking for chunks
icewind1991 Oct 7, 2015
021ed8b
adjust tests for new dav classes
icewind1991 Oct 20, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 13 additions & 8 deletions apps/dav/lib/connector/sabre/file.php
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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']);
}
}