High level file locking #16237

icewind1991 · 2015-05-11T11:47:52Z

~~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 · 2015-05-11T12:15:33Z

@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 · 2015-05-11T12:16:21Z

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 · 2015-05-11T19:27:24Z

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 · 2015-05-12T08:26:28Z

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 · 2015-05-12T08:33:28Z

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.

PVince81 · 2015-05-12T14:45:59Z

lib/private/files/storage/common.php

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


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.

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.

icewind1991 · 2015-05-12T15:11:45Z

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

PVince81 · 2015-05-12T15:21:44Z

lib/private/server.php

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


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)

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

PVince81 · 2015-05-12T15:23:53Z

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

PVince81 · 2015-05-12T15:25:13Z

lib/private/files/view.php

@@ -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);


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

PVince81 · 2015-05-12T15:30:37Z

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 · 2015-05-12T15:32:17Z

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

PVince81 · 2015-05-12T15:53:27Z

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 · 2015-05-12T16:40:39Z

@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 · 2015-05-12T16:42:30Z

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

For Sabre to be able to return the proper error code instead of 500, the LockedException is now rethrown as FileLocked exception in the Sabre connector

icewind1991 · 2015-06-01T11:42:08Z

@owncloud-bot retest this please

DeepDiver1975 · 2015-06-01T11:45:53Z

@owncloud-bot retest this please

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

scrutinizer-notifier · 2015-06-01T12:10:23Z

A new inspection was created.

icewind1991 · 2015-06-01T13:45:10Z

so everything passes on jenkins? @DeepDiver1975

icewind1991 · 2015-06-01T13:50:28Z

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

PVince81 · 2015-06-01T13:53:01Z

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

icewind1991 · 2015-06-01T13:57:21Z

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

cc @DeepDiver1975

DeepDiver1975 · 2015-06-01T14:37:23Z

so everything passes on jenkins? @DeepDiver1975

Yes - unit test are fine on this branch

DeepDiver1975 · 2015-06-01T14:37:30Z

👍

DeepDiver1975 · 2015-06-01T14:37:35Z

👍

High level file locking

PVince81 · 2015-06-01T14:50:37Z

Let me move the checkboxes into a separate ticket

DeepDiver1975 · 2015-06-01T14:51:11Z

Let me move the checkboxes into a separate ticket

awesome - THX

PVince81 · 2015-06-01T15:07:58Z

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.

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

DeepDiver1975 added the 2 - Developing label May 11, 2015

PVince81 reviewed May 12, 2015
View reviewed changes

icewind1991 mentioned this pull request May 12, 2015

dont go trough the view when renaming/copying on shared storages #16284

Merged

PVince81 reviewed May 12, 2015
View reviewed changes

PVince81 force-pushed the file-locking branch from 211ad8f to 91acd5f Compare May 13, 2015 08:59

icewind1991 and others added 10 commits June 1, 2015 13:22

use arraycache for locking in unit tests

72776b1

add locking for non-chunking webdav upload

8665a98

Convert LockedException to FileLocked in Sabre connector

ba174ac

For Sabre to be able to return the proper error code instead of 500, the LockedException is now rethrown as FileLocked exception in the Sabre connector

Move locking exceptions

0451a66

Return 423 instead of 503 for locked files

270a10b

Adding information on file locking status to admin section

43772e2

add method to atomically change between shared and exclusive lock

a1372b2

add changeLock to the storage api

661c9e2

shared lock around hooks

ce04cf6

fix nooplockingprovider

8902e2b

icewind1991 force-pushed the file-locking branch from c106d51 to 8902e2b Compare June 1, 2015 11:25

Fixing undefined index 'foo'

2104c2f

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

Merge pull request #16237 from owncloud/file-locking

2f82968

High level file locking

DeepDiver1975 merged commit 2f82968 into master Jun 1, 2015

DeepDiver1975 deleted the file-locking branch June 1, 2015 14:37

PVince81 mentioned this pull request Jun 22, 2015

Two pushes of the same file at about the same time result in no changes in the etag. #7802

Closed

lock bot locked as resolved and limited conversation to collaborators Aug 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

High level file locking #16237

High level file locking #16237

icewind1991 commented May 11, 2015

DeepDiver1975 commented May 11, 2015

DeepDiver1975 commented May 11, 2015

DeepDiver1975 commented May 11, 2015

PVince81 commented May 12, 2015

DeepDiver1975 commented May 12, 2015

PVince81 May 12, 2015

PVince81 May 12, 2015

icewind1991 commented May 12, 2015

PVince81 May 12, 2015

LukasReschke May 12, 2015

PVince81 commented May 12, 2015

PVince81 May 12, 2015

PVince81 commented May 12, 2015

PVince81 commented May 12, 2015

PVince81 commented May 12, 2015

PVince81 commented May 12, 2015

PVince81 commented May 12, 2015

icewind1991 commented Jun 1, 2015

DeepDiver1975 commented Jun 1, 2015

scrutinizer-notifier commented Jun 1, 2015

icewind1991 commented Jun 1, 2015

icewind1991 commented Jun 1, 2015

PVince81 commented Jun 1, 2015

icewind1991 commented Jun 1, 2015

DeepDiver1975 commented Jun 1, 2015

DeepDiver1975 commented Jun 1, 2015

DeepDiver1975 commented Jun 1, 2015

PVince81 commented Jun 1, 2015

DeepDiver1975 commented Jun 1, 2015

PVince81 commented Jun 1, 2015

High level file locking #16237

High level file locking #16237

Conversation

icewind1991 commented May 11, 2015

DeepDiver1975 commented May 11, 2015

DeepDiver1975 commented May 11, 2015

DeepDiver1975 commented May 11, 2015

PVince81 commented May 12, 2015

DeepDiver1975 commented May 12, 2015

PVince81 May 12, 2015

Choose a reason for hiding this comment

PVince81 May 12, 2015

Choose a reason for hiding this comment

icewind1991 commented May 12, 2015

PVince81 May 12, 2015

Choose a reason for hiding this comment

LukasReschke May 12, 2015

Choose a reason for hiding this comment

PVince81 commented May 12, 2015

PVince81 May 12, 2015

Choose a reason for hiding this comment

PVince81 commented May 12, 2015

PVince81 commented May 12, 2015

PVince81 commented May 12, 2015

PVince81 commented May 12, 2015

PVince81 commented May 12, 2015

icewind1991 commented Jun 1, 2015

DeepDiver1975 commented Jun 1, 2015

scrutinizer-notifier commented Jun 1, 2015

icewind1991 commented Jun 1, 2015

icewind1991 commented Jun 1, 2015

PVince81 commented Jun 1, 2015

icewind1991 commented Jun 1, 2015

DeepDiver1975 commented Jun 1, 2015

DeepDiver1975 commented Jun 1, 2015

DeepDiver1975 commented Jun 1, 2015

PVince81 commented Jun 1, 2015

DeepDiver1975 commented Jun 1, 2015

PVince81 commented Jun 1, 2015