Fix releasing a shared lock multiple times

[jvsalo]

This patch makes MemcacheLockingProvider handle multiple releases of the same shared lock gracefully.

Previously, the lock will get negative values that cause problems as seen in #21073

During PUT requests,

server/apps/dav/lib/Connector/Sabre/Directory.php

Line 157 in 5e35594

$node->releaseLock(ILockingProvider::LOCK_SHARED);

releases a lock once, and

server/apps/dav/lib/Connector/Sabre/LockPlugin.php

Line 83 in 5e35594

$node->releaseLock(ILockingProvider::LOCK_SHARED);

releases the same lock again (see the issue); LockPlugin never acquired the lock.

MemcacheLockingProvider already had some logic to handle multiple releases (negative going lock values), but it was subject to a race condition.

[kesselb]

Thanks for your patch 🎉

If the LockPlugin already obtains the lock why not remove the shared lock in createFile? 🤔

[jvsalo]

If the LockPlugin already obtains the lock why not remove the shared lock in createFile?

Thanks for the quick response! In my debug session, I found that LockPlugin::getLock() (beforeMethod handler) doesn't get a Node from getNodeForPath(), and as such cannot call acquireLock(). However, upon exiting the request (afterMethod handler), LockPlugin::releaseLock() does get the Node and will indeed call releaseLock().

Since it's a PUT request for a new file, perhaps it makes some sense that initially the Node is not found? But I'm very new to the codebase so I only have little idea what I'm talking about, but this I discovered by instrumenting the code yesterday.

[kesselb]

Since it's a PUT request for a new file, perhaps it makes some sense that initially the Node is not found?

That makes sense. So it's maybe enough to remove the lock release from createFile?

But making the locking provider more stable is also a good thing. I'm just trying to understand why a code path runs twice.

[jvsalo]

So it's maybe enough to remove the lock release from createFile?

In the current code I like that it's clear what part is responsible for releasing each lock - it's literally all in the same file/function.

If Directory::createFile() lock is released in LockPlugin::releaseLock(), it may be surprising to someone studying/extending the locking code?

I find it a bit confusing that LockPlugin maybe acquires some lock and maybe releases it. Perhaps LockPlugin should remember if it actually got the lock, and only try to release if it actually did get it? AFAIK this should avoid double-locking completely in the "PUT new file" case. I suppose for updating a file, double-lock would still be taken.

It seems a bit scary to me that if LockPlugin fails to get (or release) a lock, it's "ok" in all cases without printing a warning.

[MorrisJobke]

Code looks good 👍

[welcome]

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

[MorrisJobke]

/backport to stable19

[MorrisJobke]

/backport to stable18

[MorrisJobke]

/backport to stable17

[MorrisJobke]

I closed the ones to 17 and 18, because they do not apply clean.