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.

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

High level file locking #16237

Merged
merged 35 commits into from Jun 1, 2015

Conversation

Projects
None yet
7 participants
@icewind1991
Copy link
Member

icewind1991 commented May 11, 2015

Currently explodes share unit tests.

From what I can tell they explode because the view lock the files, call the shared storage, which in turn calls the view, causing the locking logic to be called again.
#12086 Should fix that but is not something we can do for 8.1

cc @PVince81 @DeepDiver1975

@DeepDiver1975

This comment has been minimized.

Copy link
Member

DeepDiver1975 commented May 11, 2015

@icewind1991

07:52:15 .....................................PHP Fatal error:  Call to undefined method Test\Files\Filesystem::mount() in /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/tests/lib/files/view.php on line 1093
07:52:21 PHP Stack trace:
07:52:21 PHP   1. {main}() /usr/local/bin/phpunit:0
07:52:21 PHP   2. PHPUnit_TextUI_Command::main() /usr/local/bin/phpunit:612
07:52:21 PHP   3. PHPUnit_TextUI_Command->run() phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:138
07:52:21 PHP   4. PHPUnit_TextUI_TestRunner->doRun() phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:186
07:52:21 PHP   5. PHPUnit_Framework_TestSuite->run() phar:///usr/local/bin/phpunit/phpunit/TextUI/TestRunner.php:423
07:52:21 PHP   6. PHPUnit_Framework_TestSuite->run() phar:///usr/local/bin/phpunit/phpunit/Framework/TestSuite.php:751
07:52:21 PHP   7. PHPUnit_Framework_TestCase->run() phar:///usr/local/bin/phpunit/phpunit/Framework/TestSuite.php:751
07:52:21 PHP   8. PHPUnit_Framework_TestResult->run() phar:///usr/local/bin/phpunit/phpunit/Framework/TestCase.php:722
07:52:21 PHP   9. PHP_Invoker->invoke() phar:///usr/local/bin/phpunit/phpunit/Framework/TestResult.php:641
07:52:21 PHP  10. call_user_func_array:{phar:///usr/local/bin/phpunit/php-invoker/Invoker.php:93}() phar:///usr/local/bin/phpunit/php-invoker/Invoker.php:93
07:52:21 PHP  11. PHPUnit_Framework_TestCase->runBare() phar:///usr/local/bin/phpunit/php-invoker/Invoker.php:93
07:52:21 PHP  12. PHPUnit_Framework_TestCase->runTest() phar:///usr/local/bin/phpunit/phpunit/Framework/TestCase.php:766
07:52:21 PHP  13. ReflectionMethod->invokeArgs() phar:///usr/local/bin/phpunit/phpunit/Framework/TestCase.php:881
07:52:21 PHP  14. Test\Files\View->testReadFromWriteLockedPath() phar:///usr/local/bin/phpunit/phpunit/Framework/TestCase.php:881

@DeepDiver1975 DeepDiver1975 added this to the 8.1-current milestone May 11, 2015

@DeepDiver1975

This comment has been minimized.

Copy link
Member

DeepDiver1975 commented May 11, 2015

From what I can tell they explode because the view lock the files, call the shared storage, which in turn calls the view, causing the locking logic to be called again.
#12086 Should fix that but is not something we can do for 8.1

@karlitschek looks like we have to talk on how to continue with this

@DeepDiver1975

This comment has been minimized.

Copy link
Member

DeepDiver1975 commented May 11, 2015

76 filing unit tests - that's a statement

    Test Result (76 failures / +76)

    Test_Util.testHomeStorageWrapperWithoutQuota
    Test_Util.testHomeStorageWrapperWithQuota
    OCA\Encryption\Tests\MigrationTest.testMigrateToNewFolderStructure
    Test_Files_Sharing_Updater.testDeleteParentFolder
    Test_Files_Sharing_Updater.testShareFile
    Test_Files_Sharing_Updater.testRename
    Test_Files_Sharing_Api.testCreateShare
    Test_Files_Sharing_Api.testCreateShareInvalidPermissions
    Test_Files_Sharing_Api.testEnfoceLinkPassword
    Test_Files_Sharing_Api.testSharePermissions
    Test_Files_Sharing_Api.testGetShareFromFolderReshares
    Test_Files_Sharing_Api.testGetShareFromSubFolderReShares
    Test_Files_Sharing_Api.testGetShareFromFolderReReShares
    Test_Files_Sharing_Api.testGetShareMultipleSharedFolder
    Test_Files_Sharing_Api.testGetShareFromFileReReShares
    Test_Files_Sharing_Api.testGetShareFromUnknownId
    Test_Files_Sharing_Api.testUpdateShareExpireDate
    Test_Files_Sharing_Api.testShareFolderWithAMountPoint
    Test_Files_Sharing_Api.testShareStorageMountPoint
    Test_Files_Sharing_Api.testShareNonExisting
    Test_Files_Sharing_Api.testShareNotOwner
    Test_Files_Sharing_Api.testDefaultExpireDate
    OCA\Files_sharing\Tests\EtagPropagation.testOwnerDeleteInShare
    OCA\Files_sharing\Tests\EtagPropagation.testOwnerDeleteInReShare
    OCA\Files_sharing\Tests\EtagPropagation.testOwnerUnshares
    OCA\Files_sharing\Tests\EtagPropagation.testRecipientUnsharesFromSelf
    OCA\Files_sharing\Tests\EtagPropagation.testRecipientWritesToShare
    OCA\Files_sharing\Tests\EtagPropagation.testRecipientWritesToReshare
    OCA\Files_sharing\Tests\EtagPropagation.testRecipientWritesToOtherRecipientsReshare
    OCA\Files_sharing\Tests\EtagPropagation.testRecipientRenameInShare
    OCA\Files_sharing\Tests\EtagPropagation.testRecipientRenameInReShare
    OCA\Files_sharing\Tests\EtagPropagation.testRecipientRenameResharedFolder
    OCA\Files_sharing\Tests\EtagPropagation.testRecipientDeleteInShare
    OCA\Files_sharing\Tests\EtagPropagation.testRecipientDeleteInReShare
    OCA\Files_sharing\Tests\EtagPropagation.testReshareRecipientWritesToReshare
    OCA\Files_sharing\Tests\EtagPropagation.testReshareRecipientRenameInReShare
    OCA\Files_sharing\Tests\EtagPropagation.testReshareRecipientDeleteInReShare
    Test_Files_Sharing_Mount::testStripUserFilesPath.testStripUserFilesPath with data set #0
    Test_Files_Sharing_Mount::testStripUserFilesPath.testStripUserFilesPath with data set #1
    Test_Files_Sharing_Mount::testStripUserFilesPath.testStripUserFilesPath with data set #2
    Test_Files_Sharing_Mount::testStripUserFilesPath.testStripUserFilesPath with data set #3
    Test_Files_Sharing_Mount::testStripUserFilesPath.testStripUserFilesPath with data set #4
    Test_Files_Sharing_Mount::testStripUserFilesPath.testStripUserFilesPath with data set #5
    Test_Files_Sharing_Mount.testShareMountLoseParentFolder
    Test_Files_Sharing_Mount.testDeleteParentOfMountPoint
    Test_Files_Sharing_Mount.testMoveSharedFile
    Test_Files_Sharing_Mount.testMoveGroupShare
    Test_Files_Sharing_Storage.testParentOfMountPointIsGone
    Test_Files_Sharing_Storage.testRenamePartFile
    Test_Files_Sharing_Storage.testFilesize
    Test_Files_Sharing_Storage.testGetPermissions
    Test_Files_Sharing_Storage.testFopenWithReadOnlyPermission
    Test_Files_Sharing_Storage.testFopenWithCreateOnlyPermission
    Test_Files_Sharing_Storage.testFopenWithUpdateOnlyPermission
    Test_Files_Sharing_Storage.testFopenWithDeleteOnlyPermission
    Test_Files_Sharing_Storage.testMountSharesOtherUser
    Test_Files_Sharing_Storage.testCopyFromStorage
    Test_Files_Sharing_Storage.testMoveFromStorage
    Test_Files_Sharing::testFileSharePermissions.testFileSharePermissions with data set #0
    Test_Files_Sharing::testFileSharePermissions.testFileSharePermissions with data set #1
    Test_Files_Sharing::testFileSharePermissions.testFileSharePermissions with data set #2
    Test_Files_Sharing::testFileSharePermissions.testFileSharePermissions with data set #3
    Test_Files_Sharing::testFileSharePermissions.testFileSharePermissions with data set #4
    Test_Files_Sharing.testUnshareFromSelf
    Test_Files_Sharing.testGroupingOfShares
    Test_Files_Sharing.testShareAndUnshareFromSelf
    Test_Files_Sharing.testShareWithDifferentShareFolder
    Test_Files_Sharing.testShareWithGroupUniqueName
    Test_Files_Sharing_Backend.testGetParents
    OCA\Files_sharing\Tests\UnshareChildren.testUnshareChildren
    OCA\Files_Sharing\Controllers\ShareControllerTest.testShowShareWithDeletedFile
    OCA\Files_Sharing\Controllers\ShareControllerTest.testDownloadShareWithDeletedFile
    Test_Trashbin.testExpireOldFiles
    Test_Trashbin.testExpireOldFilesShared
    Test_Trashbin.testExpireOldFilesUtilLimitsAreMet
    Test_Files_Versioning.testRenameInSharedFolder
@PVince81

This comment has been minimized.

Copy link
Member

PVince81 commented May 12, 2015

I guess it's everything related to sharing ? Since sharing is the one part where storages go back to view level (which shouldn't be allowed!)

@DeepDiver1975

This comment has been minimized.

Copy link
Member

DeepDiver1975 commented May 12, 2015

Since sharing is the one part where storages go back to view level (which shouldn't be allowed!)

yes - this is the root of all evil - trash and/or version do the same afaik.

This smells like bigger reengineering - time line is of question.

* @throws \OCP\Lock\LockedException
*/
public function acquireLock($path, $type, ILockingProvider $provider) {
$provider->acquireLock($this->getId() . '::' . $path, $type);

This comment has been minimized.

@PVince81

PVince81 May 12, 2015

Member

I suggest to add a utility method to create the lock ids based on getId() + path. This way it can be reused, the format is enforced and also can potentially be used for unit testing too.

This comment has been minimized.

@PVince81

PVince81 May 12, 2015

Member

I see that the provider's interface is expecting a "$path" as argument, but here what we're passing is more of a key.
Maybe at some point we should rename the locking provider's argument to "$key" then to make it more generic.

@@ -76,4 +77,18 @@ public function getStorageCache();
*/
public function getMetaData($path);
/**
* @param string $path

This comment has been minimized.

@PVince81

PVince81 May 12, 2015

Member

Please add a comment

throw new \Exception();
}
$this->fakeRoot = $root;
$this->updater = new Updater($this);
$this->lockingProvider = \OC::$server->getLockingProvider();

This comment has been minimized.

@PVince81

PVince81 May 12, 2015

Member

Do we want to inject this instead as an optional constructor argument ? Could be useful to mock the provider for unit testing. But as you already wrote tests it might not be relevant.

}
} catch (\Exception $e) {
if (in_array('write', $hooks) || in_array('delete', $hooks)) {
$this->lockFile($path, ILockingProvider::LOCK_EXCLUSIVE);

This comment has been minimized.

@PVince81

PVince81 May 12, 2015

Member

You probably meant unlockFile here ?

Would be good to have a unit test to trigger/test this.

if (!$path) {
return [];
}
$parts = explode('/', $path);

This comment has been minimized.

@PVince81

PVince81 May 12, 2015

Member

Please trim() the path to remove the leading and trailing slashes, just in case. (we had many bugs in the past in other places due to assumptions about the passed $path)

}
$parts = explode('/', $path);
// remove the singe file

This comment has been minimized.

@PVince81

PVince81 May 12, 2015

Member

"singe" in french means 🐒

private function lockPath($path, $type) {
$mount = $this->getMount($path);
if ($mount) {
$mount->getStorage()->acquireLock($mount->getInternalPath($this->getAbsolutePath($path)), $type, $this->lockingProvider);

This comment has been minimized.

@PVince81

PVince81 May 12, 2015

Member

split to multiple lines for better readability ?

return $result;
}
private function lockPath($path, $type) {

This comment has been minimized.

@PVince81

PVince81 May 12, 2015

Member

Please add a PHPDoc. Is this the $path relative to the user's home ? (files)
$type is probably lockingType ?

}
/**
* Lock a path and all it's parents

This comment has been minimized.

@PVince81

PVince81 May 12, 2015

Member

it's => its 😉 (sorry...)

This comment has been minimized.

@PVince81

PVince81 May 12, 2015

Member

"all its parents regardless of storage boundaries" ? I mean we do not stop at storage boundaries because it's an absolute path

* @param int $type
*/
public function lockFile($path, $type) {
$path = rtrim($path, '/');

This comment has been minimized.

@PVince81

PVince81 May 12, 2015

Member

Do we expect the path to have a leading slash too ?
Better do $path = '/' . trim($path, '/') just to be sure.

@icewind1991

This comment has been minimized.

Copy link
Member Author

icewind1991 commented May 12, 2015

Found an easier way to not go trough the view from the shared storage without doing the full refactor (which we still should do for 8.1): #16284

Trash has a similar issue, will make separate PR for that

@@ -420,6 +421,13 @@ public function __construct($webRoot) {
$this->getLogger()
);
});
$this->registerService('LockingProvider', function (Server $c) {
/** @var \OC\Memcache\Factory $memcacheFactory */
$memcacheFactory = $c->getMemCacheFactory();

This comment has been minimized.

@PVince81

PVince81 May 12, 2015

Member

We'll need the config switch to disable this by default. Maybe provide a NoopLockingProvider ?

And also what happens if memcache isn't available/configured ? We should probably disable locking in that case. (we could add a warning on the admin page later if the config says "I want locking" but memcache is missing)

This comment has been minimized.

@LukasReschke

LukasReschke May 12, 2015

Member

Also see #11804 (comment) for my and @butonic's considerations of using a memory cache that does not guarantee availability. MemcacheFactory uses the distributed cache which can also be memcached

@@ -413,4 +414,19 @@ public function copyFromStorage(\OCP\Files\Storage $sourceStorage, $sourceIntern
* @since 8.1.0
*/
public function moveFromStorage(\OCP\Files\Storage $sourceStorage, $sourceInternalPath, $targetInternalPath);
/**
* @param string $path

This comment has been minimized.

@PVince81

PVince81 May 12, 2015

Member

Ah, this is the interface, so it should have a proper PHPDoc to explain what this is about 😄

@PVince81

This comment has been minimized.

Copy link
Member

PVince81 commented May 12, 2015

  • add more unit tests to cover more code paths (I see we have conditionals based on what kind of hooks are registered, so we should somehow cover that). Maybe have a mock LockingProvider that just checks whether the lock is properly set (or not) when such conditions like hooks are met
@@ -629,6 +637,9 @@ public function rename($path1, $path2) {
$internalPath1 = $mount1->getInternalPath($absolutePath1);
$internalPath2 = $mount2->getInternalPath($absolutePath2);
$this->lockFile($path1, ILockingProvider::LOCK_EXCLUSIVE);
$this->lockFile($path2, ILockingProvider::LOCK_EXCLUSIVE);

This comment has been minimized.

@PVince81

PVince81 May 12, 2015

Member

If say we already locked $path1, but $path2 cannot be locked. We'll probably get an exception ? We should unlock $path1 in the exception/finally handler

if ($internalPath1 === '' and $mount1 instanceof MoveableMount) {
// since $path2 now points to a different storage we need to unlock the path on the old storage separately
$storage2->releaseLock($internalPath2, ILockingProvider::LOCK_EXCLUSIVE, $this->lockingProvider);

This comment has been minimized.

@PVince81

PVince81 May 12, 2015

Member

Do we still need to call $this->unlockFile($path2) then ? Or should it be done in the else block instead ?

This comment has been minimized.

@icewind1991

icewind1991 May 22, 2015

Author Member

still needs to be called to handle the parent folders

@PVince81

This comment has been minimized.

Copy link
Member

PVince81 commented May 12, 2015

How about files outside of "files", will they be locked too as per #11804 (comment) ? And exclude "cache" and "files_encryption" maybe.
A method shouldLock($path) could help with that.

@PVince81

This comment has been minimized.

Copy link
Member

PVince81 commented May 12, 2015

I already ticked a few of the items this PR covers here: #11804 (comment)

@PVince81

This comment has been minimized.

Copy link
Member

PVince81 commented May 12, 2015

  • fallback for when memcache is disabled (I'm currently getting "Call to undefined method OC\Memcache\Null::inc() at /srv/www/htdocs/owncloud/lib/private/lock/memcachelockingprovider.php#64")
  • do the unit tests need memcache too ? should we add a skip or warning when memcache is not available to tell that testing is incomplete ?
@PVince81

This comment has been minimized.

Copy link
Member

PVince81 commented May 12, 2015

@icewind1991 I enabled memcached locally, then merged #16284 into this branch locally, ran the unit tests: a lot of sharing tests still fail when running everything, but pass when run individually.

I think there might still be some tests that do not clean up properly. Maybe we need to add a "clear all locks" in the TestCase's tearDown.

@PVince81

This comment has been minimized.

Copy link
Member

PVince81 commented May 12, 2015

either this or provide a mock locking provider that just caches key/values locally, and provides a method to clear itself.

@PVince81 PVince81 force-pushed the file-locking branch from 211ad8f to 91acd5f May 13, 2015

@icewind1991 icewind1991 force-pushed the file-locking branch from c106d51 to 8902e2b Jun 1, 2015

@icewind1991

This comment has been minimized.

Copy link
Member Author

icewind1991 commented Jun 1, 2015

@owncloud-bot retest this please

@DeepDiver1975

This comment has been minimized.

Copy link
Member

DeepDiver1975 commented Jun 1, 2015

@owncloud-bot retest this please

@icewind1991 this is pointless - jenkins pull request analyser cannot recover ... in addition unit test fail

@scrutinizer-notifier

This comment has been minimized.

Copy link

scrutinizer-notifier commented Jun 1, 2015

A new inspection was created.

@icewind1991

This comment has been minimized.

Copy link
Member Author

icewind1991 commented Jun 1, 2015

so everything passes on jenkins? @DeepDiver1975

@icewind1991

This comment has been minimized.

Copy link
Member Author

icewind1991 commented Jun 1, 2015

@PVince81 afaik there are no regressions in the TODO's you listen, shall we merge this and take care of those separately?

@PVince81

This comment has been minimized.

Copy link
Member

PVince81 commented Jun 1, 2015

I was ok to merge this a while ago: #16237 (comment)

@icewind1991

This comment has been minimized.

Copy link
Member Author

icewind1991 commented Jun 1, 2015

Ok, can we have the final +1 with the latest changes (ce04cf6 mostly)

cc @DeepDiver1975

@DeepDiver1975

This comment has been minimized.

Copy link
Member

DeepDiver1975 commented Jun 1, 2015

so everything passes on jenkins? @DeepDiver1975

Yes - unit test are fine on this branch

@DeepDiver1975

This comment has been minimized.

Copy link
Member

DeepDiver1975 commented Jun 1, 2015

👍

1 similar comment
@DeepDiver1975

This comment has been minimized.

Copy link
Member

DeepDiver1975 commented Jun 1, 2015

👍

DeepDiver1975 added a commit that referenced this pull request Jun 1, 2015

@DeepDiver1975 DeepDiver1975 merged commit 2f82968 into master Jun 1, 2015

4 of 5 checks passed

default Build finished.
Details
server-master-linux-locking/database=mysql,label=SLAVE Build #5 succeeded in 41 min
Details
server-master-linux-locking/database=oci,label=SLAVE Build #5 succeeded in 1 hr 2 min
Details
server-master-linux-locking/database=pgsql,label=SLAVE Build #5 succeeded in 48 min
Details
server-master-linux-locking/database=sqlite,label=SLAVE Build #5 succeeded in 18 min
Details

@DeepDiver1975 DeepDiver1975 deleted the file-locking branch Jun 1, 2015

@PVince81

This comment has been minimized.

Copy link
Member

PVince81 commented Jun 1, 2015

Let me move the checkboxes into a separate ticket

@DeepDiver1975

This comment has been minimized.

Copy link
Member

DeepDiver1975 commented Jun 1, 2015

Let me move the checkboxes into a separate ticket

awesome - THX

@PVince81

This comment has been minimized.

Copy link
Member

PVince81 commented Jun 1, 2015

Raised here: #16664

I didn't raise separate tickets yet because many already exist as ticket, I added them to that "summary" ticket. I also moved the tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment