Skip to content

Optimize PUT - don't fetch and update checksum again, reunse the one from part file #27532

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

Merged
PVince81 merged 1 commit into from
Jul 5, 2017
Merged

Optimize PUT - don't fetch and update checksum again, reunse the one from part file #27532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 0 additions & 12 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 @@ -243,18 +243,6 @@ public function put($data) {
}

$this->refreshInfo();

$meta = $partStorage->getMetaData($internalPartPath);

if (isset($meta['checksum'])) {
$this->fileView->putFileInfo(
$this->path,
['checksum' => $meta['checksum']]
);
}

$this->refreshInfo();

} catch (StorageNotAvailableException $e) {
throw new ServiceUnavailable("Failed to check file size: " . $e->getMessage());
}
Expand Down
2 changes: 0 additions & 2 deletions lib/private/Files/Cache/Scanner.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,8 +201,6 @@ public function scanFile($file, $reuseExisting = 0, $parentId = -1, $cacheData =
$fileId = -1;
}
if (!empty($newData)) {
// Reset the checksum if the data has changed
$newData['checksum'] = '';
Copy link
Contributor Author

@mrow4a mrow4a May 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to me some kind of workaround for some purpose, probably for what I have erased to work

Copy link
Member

@DeepDiver1975 DeepDiver1975 May 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from my understanding this is necessary if the file was changed on disk which is detected by the scanner.
in this case the checksum has to be removed

Copy link
Contributor Author

@mrow4a mrow4a May 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just went with debugger, first time the PUT gets there is with

core/apps/dav/lib/Connector/Sabre/File.php

Line 204 in 186595c

$storage->getUpdater()->update($internalPath);
, and

core/lib/private/Files/Cache/Scanner.php

Line 204 in 186595c

$data['fileid'] = $this->addToCache($file, $newData, $fileId);
$newData has following contents
selection_289

Next time it gets there is in

core/apps/dav/lib/Connector/Sabre/File.php

Line 215 in 186595c

if ($this->fileView->touch($this->path, $request->server['HTTP_X_OC_MTIME'])) {
and it has following content
selection_290

Copy link
Contributor Author

@mrow4a mrow4a May 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DeepDiver1975 any thoughts?

Copy link
Contributor Author

@mrow4a mrow4a May 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot think where cleaning of checksum might be usefull, first call goes through

core/lib/private/Files/Cache/Scanner.php

Line 200 in 186595c

$newData = $data;
, and second goes through

core/lib/private/Files/Cache/Scanner.php

Line 180 in 186595c

if (empty($cacheData['etag'])) {

Copy link
Contributor

@PVince81 PVince81 May 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you retest with an external storage ? Then change a file directly on external storage and run occ files:scan. In this case, the checksum in oc_filecache must disappear since the file changed and we didn't recompute it yet.

If this still works correctly then maybe there is yet another code paths that achieves the same result... strange

Copy link
Contributor

@PVince81 PVince81 May 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also beware, when testing uploads, always test these:

  • old endpoint, new file upload
  • old endpoint, upload + overwrite
  • old endpoint, new file chunked upload
  • old endpoint, chunked upload + overwrite
  • repeat above with new endpoint

Copy link
Contributor

@PVince81 PVince81 Jun 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please make sure these are covered by integration tests + checksums to make sure this PR doesn't break anything

$data['fileid'] = $this->addToCache($file, $newData, $fileId);
}
if (isset($cacheData['size'])) {
Expand Down
38 changes: 37 additions & 1 deletion lib/private/Files/Stream/Checksum.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,11 +133,47 @@ private function updateHashingContexts($data) {
}

/**
* Remove .part extension from a file path
* @param string $path Path that may identify a .part file
* @return string File path without .part extension
*/
private function stripPartialFileExtension($path) {
$extension = pathinfo($path, PATHINFO_EXTENSION);

if ( $extension === 'part') {

$newLength = strlen($path) - 5; // 5 = strlen(".part")
$fPath = substr($path, 0, $newLength);

// if path also contains a transaction id, we remove it too
$extension = pathinfo($fPath, PATHINFO_EXTENSION);
if(substr($extension, 0, 12) === 'ocTransferId') { // 12 = strlen("ocTransferId")
$newLength = strlen($fPath) - strlen($extension) -1;
$fPath = substr($fPath, 0, $newLength);
}
return $fPath;

} else {
return $path;
}
}

/**
* Make checksums available for part files and the original file for which part file has been created
* @return bool
*/
public function stream_close() {
$currentPath = $this->getPathFromStreamContext();
self::$checksums[$currentPath] = $this->finalizeHashingContexts();
$checksum = $this->finalizeHashingContexts();
self::$checksums[$currentPath] = $checksum;

// If current path belongs to part file, save checksum for original file
// As a result, call to getChecksums for original file (of this part file) will
// fetch checksum from cache
$originalFilePath = $this->stripPartialFileExtension($currentPath);
if ($originalFilePath !== $currentPath){
self::$checksums[$originalFilePath] = $checksum;
}
Copy link
Contributor Author

@mrow4a mrow4a May 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will "cache" checksum for the original file (for which .part file is created)


return parent::stream_close();
}
Expand Down
43 changes: 34 additions & 9 deletions tests/lib/Files/ViewTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2466,24 +2466,49 @@ public function testGetDirectoryContentMimeFilter($filter, $expected) {
$this->assertEquals($expected, $files);
}

private function calculateChecksums($data) {
// Get hashing algorithms and calculate checksum for given data
$checksumString = '';
$checksums['sha1'] = hash('sha1', $data);
$checksums['md5'] = hash('md5', $data);
$checksums['adler32'] = hash('adler32', $data);
foreach ($checksums as $algo => $checksum) {
$checksumString .= sprintf('%s:%s ', strtoupper($algo), $checksum);
}
return rtrim($checksumString);
}

public function testFilePutContentsClearsChecksum() {
$storage = new Temporary([]);
$scanner = $storage->getScanner();
$storage->file_put_contents('foo.txt', 'bar');
Filesystem::mount($storage, [], '/test/');
$storage->mkdir('files');
$storage->file_put_contents('files/foo.txt', 'bar');
Filesystem::mount($storage, [], '/files');
$scanner->scan('');

$view = new View('/test/foo.txt');
$view->putFileInfo('.', ['checksum' => '42']);
// Obtain view on the file and check content + checksum
// If we insert file using $storage->file_put_contents checksum is not being calculated (null)
$view = new View('/files');
$this->assertEquals('bar', $view->file_get_contents('files/foo.txt'));
$data = $view->getFileInfo('files/foo.txt');
$this->assertNull($data->getChecksum());

$this->assertEquals('bar', $view->file_get_contents(''));
// Force some checksum in the file and verify
$view->putFileInfo('files/foo.txt', ['checksum' => '42']);
$data = $view->getFileInfo('files/foo.txt');
$checksum = $data->getChecksum();
$this->assertEquals('42', $checksum);

// Now use Wrapper/Checksum to calculate checksum for us (normal PUT case)
$fh = tmpfile();
fwrite($fh, 'fooo');
rewind($fh);
$view->file_put_contents('', $fh);
$this->assertEquals('fooo', $view->file_get_contents(''));
$data = $view->getFileInfo('.');
$this->assertEquals('', $data->getChecksum());
$view->file_put_contents('files/foo.txt', $fh);
$this->assertEquals('fooo', $view->file_get_contents('files/foo.txt'));
$data = $view->getFileInfo('files/foo.txt');
$checksum = $data->getChecksum();
$expectedChecksum = $this->calculateChecksums('fooo');
$this->assertEquals($expectedChecksum, $checksum);
}

public function testDeleteGhostFile() {
Expand Down