Unlock when promoting to exclusive lock fails #23741
Conversation
|
Hello @Ashod, thank you for your PR, is this ready for review, you can use labels to show the status of this PR. |
skjnldsv
requested review from
MorrisJobke,
blizzz,
nickvergessen and
rullzer
Yes @GretaD, it is ready for review and does resolve the issue for me. However, the maintainers might want to rework that function, such that there is a try/catch/finally block that does a single unlock in finally. I didn't do that, even though it's cleaner and catches other exceptions from other functions too, because I didn't want to refactor the function. My patch is the minimal change to fix the issue, not necessarily the most elegant. Also, I suspect there are other similar bugs in the code. There are other places where the changeLock function is called without catching the exception. I didn't do an exhaustive search/investigation of those cases (probably not all need to unlock the shared lock on exception), but it's worth reviewing them, now that we know this pattern is problematic. Let me know if I need to do anything further please. |
icewind1991
left a comment
icewind1991
left a comment
There was a problem hiding this comment.
makes sense.
I'm not sure if using a finally here makes sense, since it's not always the same lock type that needs to be released.
|
@Ashod could you rebase your pr on latest master? |
|
/backport to stable20 |
|
/backport to stable19 |
|
/backport to stable18 |
In certain cases changeLock to EXCLUSIVE fails and throws LockedException. This leaves the file locked as SHARED in file_put_contents, which prevents retrying (because on second call file_put_contents takes another SHARED lock on the same file, and changeLock doesn't allow more than a single SHARED lock to promote to EXCLUSIVE). To avoid this case, we catch the LockedException and unlock before re-throwing. Signed-off-by: Ashod Nakashian <[email protected]>
@icewind1991, that's a design decision, and it's fine (by me) not to use finally. But the It's true that between these two call there could be other locks, but those other locks need to be managed in their own right (like the Exclusive lock), such that at the point of returning from the function the state would be restored to what it was at the beginning. In other words, the lock state is always symmetric between the lock and unlock calls. And try/catch/finally is designed exactly for this use-case in mind. It can of course be nested, and any sub-section that has its own locks/logic would use finally to make sure it restores the state it inherited at the start of its block (so we could have two try/finally blocks in this function with my patch, the outer on for the Shared and the inner one for the Exclusive lock). In C++ this exact same pattern is implemented trivially by a class that takes the lock in its constructor and releases in its destructor, regardless of what happens in between. Of course you could have functions that aren't symmetric, where you return with a different lock-state than you entered with. In those cases one shouldn't use finally to unlock (obviously), but those functions are quite rare (they are probably functions that implement the locking mechanisms itself) and not consumers of the lock. At any rate, it's not up to me to decide. Just noting that there are many corner-cases that potentially leak the locks in the NC codebase. And not having a catch-all finally in those functions means these functions are forever prone to break after a trivial change that moves things around, or even when an unrelated function throws an exception that all the functions that lock don't catch and handle. It's better to standardize the management of locks everywhere with simple patterns that is easy to implement, maintain, and review. Leaking locks is a very serious problem, needless to say, so well worth the time investment. |
|
FYI - we have to have a sleep(2) for our Nextcloud AppImage to try to avoid this race, and we're -really- eager to get rid of that ASAP =) |
|
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 |
|
/backport to stable19 |
|
/backport to stable18 |
9 participants
In certain cases changeLock to EXCLUSIVE fails
and throws LockedException. This leaves the
file locked as SHARED in file_put_contents,
which prevents retrying (because on second
call file_put_contents takes another SHARED
lock on the same file, and changeLock doesn't
allow more than a single SHARED lock to promote
to EXCLUSIVE).
To avoid this case, we catch the LockedException
and unlock before re-throwing.
Resolves #23740