Skip to content

Commit

Permalink
Merge pull request #36788 from nextcloud/revert-36589-enh/perf-remove…
Browse files Browse the repository at this point in the history 
…-icache

Revert "fix(performance): Do not set up filesystem on every call"
  • Loading branch information
@miaulalala
miaulalala authored Feb 21, 2023
2 parents 93e703b + 98ed72b commit 13ef475
Show file tree
Hide file tree
Showing 30 changed files with 1,079 additions and 159 deletions.
75 changes: 44 additions & 31 deletions apps/dav/lib/Connector/Sabre/Directory.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@
use OCP\Lock\ILockingProvider;
use OCP\Lock\LockedException;
use Psr\Log\LoggerInterface;
use Sabre\DAV\Exception;
use Sabre\DAV\Exception\BadRequest;
use Sabre\DAV\Exception\Locked;
use Sabre\DAV\Exception\NotFound;
Expand Down Expand Up @@ -103,19 +102,33 @@ public function __construct(View $view, FileInfo $info, ?CachingTree $tree = nul
* @param string $name Name of the file
* @param resource|string $data Initial payload
* @return null|string
* @throws Exception\EntityTooLarge
* @throws Exception\UnsupportedMediaType
* @throws FileLocked
* @throws InvalidPath
* @throws Exception
* @throws BadRequest
* @throws Exception\Forbidden
* @throws ServiceUnavailable
* @throws \Sabre\DAV\Exception
* @throws \Sabre\DAV\Exception\BadRequest
* @throws \Sabre\DAV\Exception\Forbidden
* @throws \Sabre\DAV\Exception\ServiceUnavailable
*/
public function createFile($name, $data = null) {
try {
// For non-chunked upload it is enough to check if we can create a new file
if (!$this->fileView->isCreatable($this->path)) {
throw new Exception\Forbidden();
// for chunked upload also updating a existing file is a "createFile"
// because we create all the chunks before re-assemble them to the existing file.
if (isset($_SERVER['HTTP_OC_CHUNKED'])) {

// exit if we can't create a new file and we don't updatable existing file
$chunkInfo = \OC_FileChunking::decodeName($name);
if (!$this->fileView->isCreatable($this->path) &&
!$this->fileView->isUpdatable($this->path . '/' . $chunkInfo['name'])
) {
throw new \Sabre\DAV\Exception\Forbidden();
}
} else {
// For non-chunked upload it is enough to check if we can create a new file
if (!$this->fileView->isCreatable($this->path)) {
throw new \Sabre\DAV\Exception\Forbidden();
}
}

$this->fileView->verifyPath($this->path, $name);
Expand All @@ -140,8 +153,8 @@ public function createFile($name, $data = null) {
$this->fileView->unlockFile($path . '.upload.part', ILockingProvider::LOCK_EXCLUSIVE);
$node->releaseLock(ILockingProvider::LOCK_SHARED);
return $result;
} catch (StorageNotAvailableException $e) {
throw new ServiceUnavailable($e->getMessage());
} catch (\OCP\Files\StorageNotAvailableException $e) {
throw new \Sabre\DAV\Exception\ServiceUnavailable($e->getMessage(), $e->getCode(), $e);
} catch (InvalidPathException $ex) {
throw new InvalidPath($ex->getMessage(), false, $ex);
} catch (ForbiddenException $ex) {
Expand All @@ -157,22 +170,22 @@ public function createFile($name, $data = null) {
* @param string $name
* @throws FileLocked
* @throws InvalidPath
* @throws Exception\Forbidden
* @throws ServiceUnavailable
* @throws \Sabre\DAV\Exception\Forbidden
* @throws \Sabre\DAV\Exception\ServiceUnavailable
*/
public function createDirectory($name) {
try {
if (!$this->info->isCreatable()) {
throw new Exception\Forbidden();
throw new \Sabre\DAV\Exception\Forbidden();
}

$this->fileView->verifyPath($this->path, $name);
$newPath = $this->path . '/' . $name;
if (!$this->fileView->mkdir($newPath)) {
throw new Exception\Forbidden('Could not create directory ' . $newPath);
throw new \Sabre\DAV\Exception\Forbidden('Could not create directory ' . $newPath);
}
} catch (StorageNotAvailableException $e) {
throw new ServiceUnavailable($e->getMessage());
} catch (\OCP\Files\StorageNotAvailableException $e) {
throw new \Sabre\DAV\Exception\ServiceUnavailable($e->getMessage());
} catch (InvalidPathException $ex) {
throw new InvalidPath($ex->getMessage());
} catch (ForbiddenException $ex) {
Expand All @@ -190,7 +203,7 @@ public function createDirectory($name) {
* @return \Sabre\DAV\INode
* @throws InvalidPath
* @throws \Sabre\DAV\Exception\NotFound
* @throws ServiceUnavailable
* @throws \Sabre\DAV\Exception\ServiceUnavailable
*/
public function getChild($name, $info = null) {
if (!$this->info->isReadable()) {
Expand All @@ -203,12 +216,12 @@ public function getChild($name, $info = null) {
try {
$this->fileView->verifyPath($this->path, $name);
$info = $this->fileView->getFileInfo($path);
} catch (StorageNotAvailableException $e) {
throw new ServiceUnavailable($e->getMessage());
} catch (\OCP\Files\StorageNotAvailableException $e) {
throw new \Sabre\DAV\Exception\ServiceUnavailable($e->getMessage());
} catch (InvalidPathException $ex) {
throw new InvalidPath($ex->getMessage());
} catch (ForbiddenException $e) {
throw new Exception\Forbidden();
throw new \Sabre\DAV\Exception\Forbidden();
}
}

Expand Down Expand Up @@ -285,17 +298,17 @@ public function childExists($name) {
*
* @return void
* @throws FileLocked
* @throws Exception\Forbidden
* @throws \Sabre\DAV\Exception\Forbidden
*/
public function delete() {
if ($this->path === '' || $this->path === '/' || !$this->info->isDeletable()) {
throw new Exception\Forbidden();
throw new \Sabre\DAV\Exception\Forbidden();
}

try {
if (!$this->fileView->rmdir($this->path)) {
// assume it wasn't possible to remove due to permission issue
throw new Exception\Forbidden();
throw new \Sabre\DAV\Exception\Forbidden();
}
} catch (ForbiddenException $ex) {
throw new Forbidden($ex->getMessage(), $ex->getRetry());
Expand Down Expand Up @@ -330,7 +343,7 @@ public function getQuotaInfo() {
} catch (\OCP\Files\NotFoundException $e) {
$logger->warning("error while getting quota into", ['exception' => $e]);
return [0, 0];
} catch (StorageNotAvailableException $e) {
} catch (\OCP\Files\StorageNotAvailableException $e) {
$logger->warning("error while getting quota into", ['exception' => $e]);
return [0, 0];
} catch (NotPermittedException $e) {
Expand Down Expand Up @@ -362,7 +375,7 @@ public function getQuotaInfo() {
* @throws ServiceUnavailable
* @throws Forbidden
* @throws FileLocked
* @throws Exception\Forbidden
* @throws \Sabre\DAV\Exception\Forbidden
*/
public function moveInto($targetName, $fullSourcePath, INode $sourceNode) {
if (!$sourceNode instanceof Node) {
Expand All @@ -386,7 +399,7 @@ public function moveInto($targetName, $fullSourcePath, INode $sourceNode) {
// at getNodeForPath we also check the path for isForbiddenFileOrDir
// with that we have covered both source and destination
if ($sourceNode instanceof Directory && $targetNodeExists) {
throw new Exception\Forbidden('Could not copy directory ' . $sourceNode->getName() . ', target exists');
throw new \Sabre\DAV\Exception\Forbidden('Could not copy directory ' . $sourceNode->getName() . ', target exists');
}

[$sourceDir,] = \Sabre\Uri\split($sourceNode->getPath());
Expand All @@ -407,19 +420,19 @@ public function moveInto($targetName, $fullSourcePath, INode $sourceNode) {
if ($targetNodeExists || $sameFolder) {
// note that renaming a share mount point is always allowed
if (!$this->fileView->isUpdatable($destinationDir) && !$isMovableMount) {
throw new Exception\Forbidden();
throw new \Sabre\DAV\Exception\Forbidden();
}
} else {
if (!$this->fileView->isCreatable($destinationDir)) {
throw new Exception\Forbidden();
throw new \Sabre\DAV\Exception\Forbidden();
}
}

if (!$sameFolder) {
// moving to a different folder, source will be gone, like a deletion
// note that moving a share mount point is always allowed
if (!$this->fileView->isDeletable($sourcePath) && !$isMovableMount) {
throw new Exception\Forbidden();
throw new \Sabre\DAV\Exception\Forbidden();
}
}

Expand All @@ -432,7 +445,7 @@ public function moveInto($targetName, $fullSourcePath, INode $sourceNode) {

$renameOkay = $this->fileView->rename($sourcePath, $destinationPath);
if (!$renameOkay) {
throw new Exception\Forbidden('');
throw new \Sabre\DAV\Exception\Forbidden('');
}
} catch (StorageNotAvailableException $e) {
throw new ServiceUnavailable($e->getMessage());
Expand All @@ -452,7 +465,7 @@ public function copyInto($targetName, $sourcePath, INode $sourceNode) {
$sourcePath = $sourceNode->getPath();

if (!$this->fileView->isCreatable($this->getPath())) {
throw new Exception\Forbidden();
throw new \Sabre\DAV\Exception\Forbidden();
}

try {
Expand Down
135 changes: 135 additions & 0 deletions apps/dav/lib/Connector/Sabre/File.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,15 @@ public function put($data) {
// verify path of the target
$this->verifyPath();

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

/** @var Storage $partStorage */
[$partStorage] = $this->fileView->resolvePath($this->path);
$needsPartFile = $partStorage->needsPartFile() && (strlen($this->path) > 1);
Expand Down Expand Up @@ -568,6 +577,132 @@ public function getDirectDownload() {
return $storage->getDirectDownload($internalPath);
}

/**
* @param resource $data
* @return null|string
* @throws Exception
* @throws BadRequest
* @throws NotImplemented
* @throws ServiceUnavailable
*/
private function createFileChunked($data) {
[$path, $name] = \Sabre\Uri\split($this->path);

$info = \OC_FileChunking::decodeName($name);
if (empty($info)) {
throw new NotImplemented($this->l10n->t('Invalid chunk name'));
}

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

//detect aborted upload
if (isset($_SERVER['REQUEST_METHOD']) && $_SERVER['REQUEST_METHOD'] === 'PUT') {
if (isset($_SERVER['CONTENT_LENGTH'])) {
$expected = (int)$_SERVER['CONTENT_LENGTH'];
if ($bytesWritten !== $expected) {
$chunk_handler->remove($info['index']);
throw new BadRequest(
$this->l10n->t(
'Expected filesize of %1$s but read (from Nextcloud client) and wrote (to Nextcloud storage) %2$s. Could either be a network problem on the sending side or a problem writing to the storage on the server side.',
[
$this->l10n->n('%n byte', '%n bytes', $expected),
$this->l10n->n('%n byte', '%n bytes', $bytesWritten),
],
)
);
}
}
}

if ($chunk_handler->isComplete()) {
/** @var Storage $storage */
[$storage,] = $this->fileView->resolvePath($path);
$needsPartFile = $storage->needsPartFile();
$partFile = null;

$targetPath = $path . '/' . $info['name'];
/** @var \OC\Files\Storage\Storage $targetStorage */
[$targetStorage, $targetInternalPath] = $this->fileView->resolvePath($targetPath);

$exists = $this->fileView->file_exists($targetPath);

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

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

if ($needsPartFile) {
// we first assembly the target file as a part file
$partFile = $this->getPartFileBasePath($path . '/' . $info['name']) . '.ocTransferId' . $info['transferid'] . '.part';
/** @var \OC\Files\Storage\Storage $targetStorage */
[$partStorage, $partInternalPath] = $this->fileView->resolvePath($partFile);


$chunk_handler->file_assemble($partStorage, $partInternalPath);

// here is the final atomic rename
$renameOkay = $targetStorage->moveFromStorage($partStorage, $partInternalPath, $targetInternalPath);
$fileExists = $targetStorage->file_exists($targetInternalPath);
if ($renameOkay === false || $fileExists === false) {
\OC::$server->get(LoggerInterface::class)->error('\OC\Files\Filesystem::rename() failed', ['app' => 'webdav']);
// 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;
$targetStorage->unlink($targetInternalPath);
}
$this->fileView->changeLock($targetPath, ILockingProvider::LOCK_SHARED);
throw new Exception($this->l10n->t('Could not rename part file assembled from chunks'));
}
} else {
// assemble directly into the final file
$chunk_handler->file_assemble($targetStorage, $targetInternalPath);
}

// allow sync clients to send the mtime along in a header
if (isset($this->request->server['HTTP_X_OC_MTIME'])) {
$mtime = $this->sanitizeMtime($this->request->server['HTTP_X_OC_MTIME']);
if ($targetStorage->touch($targetInternalPath, $mtime)) {
$this->header('X-OC-MTime: accepted');
}
}

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

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

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

// FIXME: should call refreshInfo but can't because $this->path is not the of the final file
$info = $this->fileView->getFileInfo($targetPath);

if (isset($this->request->server['HTTP_OC_CHECKSUM'])) {
$checksum = trim($this->request->server['HTTP_OC_CHECKSUM']);
$this->fileView->putFileInfo($targetPath, ['checksum' => $checksum]);
} elseif ($info->getChecksum() !== null && $info->getChecksum() !== '') {
$this->fileView->putFileInfo($this->path, ['checksum' => '']);
}

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

return $info->getEtag();
} catch (\Exception $e) {
if ($partFile !== null) {
$targetStorage->unlink($targetInternalPath);
}
$this->convertToSabreException($e);
}
}

return null;
}

/**
* Convert the given exception to a SabreException instance
*
Expand Down
9 changes: 9 additions & 0 deletions apps/dav/lib/Connector/Sabre/FilesPlugin.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -577,6 +577,15 @@ public function handleUpdateProperties($path, PropPatch $propPatch) {
* @throws \Sabre\DAV\Exception\BadRequest
*/
public function sendFileIdHeader($filePath, \Sabre\DAV\INode $node = null) {
// chunked upload handling
if (isset($_SERVER['HTTP_OC_CHUNKED'])) {
[$path, $name] = \Sabre\Uri\split($filePath);
$info = \OC_FileChunking::decodeName($name);
if (!empty($info)) {
$filePath = $path . '/' . $info['name'];
}
}

// we get the node for the given $filePath here because in case of afterCreateFile $node is the parent folder
if (!$this->server->tree->nodeExists($filePath)) {
return;
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 @@ -61,7 +61,7 @@ public function initialize(\Sabre\DAV\Server $server) {
public function getLock(RequestInterface $request) {
// we can't 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 @@ -84,7 +84,7 @@ public function releaseLock(RequestInterface $request) {
if ($this->isLocked === false) {
return;
}
if ($request->getMethod() !== 'PUT') {
if ($request->getMethod() !== 'PUT' || isset($_SERVER['HTTP_OC_CHUNKED'])) {
return;
}
try {
Expand Down
Loading

0 comments on commit 13ef475

Please sign in to comment.