restore shared lock ttl to previous value when releasing

[icewind1991]

With shared locks, each time the lock is acquired, the ttl for the path is reset.

Due to this "ttl extension" when a shared lock isn't freed correctly for any reason
the lock won't expire until no shared locks are required for the path for 1h.
This can lead to a client repeatedly trying to upload a file, and failing forever
because the lock never gets the opportunity to expire.

To help the lock expire in this case, we lower the TTL back to what it was before we
took the shared lock only if nobody else got a shared lock after we did.

This doesn't handle all cases where multiple requests are acquiring shared locks
but it should handle some of the more common ones and not hurt things further.

Ideally each copy of a shared lock will have it's own independent TTL so locks can expire while others are still being help, but we don't have that luxury.

To test:

• setup an instance with redis file locking without applying this patch
• set a breakpoint at acquireLock
• upload a file called test.txt
• note the $path and $this->memcache->prefix used for the lock for test.txt (check $readablePath to ensure you're look at the right lock)
• remove the breakpoint
• delete test.txt
• open a redis-cli and run
  • set "<$this->memcache->prefix><$path>" 1 (key should be something like bc2af67f80d0445df3c328c2a0f08faf/lockfiles/0ca47d1e59d0cbf07f715297a70f8417
  • expire "<$this->memcache->prefix><$path>" 3600
• you have now created a "leaked" lock with a TTL of 1 hour
• wait a few seconds and run expire "<$this->memcache->prefix><$path>" in redis-cli to check that the TTL is counting down
• try to upload test.txt again, this should fail due to the leaked lock
• check the TTL of the leaked lock which should have been reset to ~1h
• apply the patch
• try uploading `test.txt again
• check that the TTL of the leaked lock didn't get reset to 1h

[come-nc]

I am not sure I understand the problem description.
When uploading test.txt fails, it is because of the existing lock? If so, it means it fails to acquire the lock, no?
If it succeed to acquire the lock and extends it, what fails?

[icewind1991]

When uploading test.txt fails, it is because of the existing lock?

Yes

If so, it means it fails to acquire the lock, no?

on upload it first 1) acquires a shared lock, 2) changes it to an exclusive one, 3) changes it back to a shared one
2) fails here, but 1) still extended the TTL

So if a client repeatedly attempts to upload a file that has a leaked shared lock, it will never succeed to change it's shared lock into an exclusive one, but the TTL will never expire.

[come-nc]

Why is the restoreTTL from changeLock not enough, given the scenario you described?
What is the rationale behind this one in releaseLock? For other kind of exceptions/failures?

[icewind1991]

We still want to restore the ttl when a request just acquires + releases.

[blizzz]

Logically it looks okay and I can follow your outlines, but I really cannot pretend to be very knowledgeable here. But contributed some bikeshedding ;)

[icewind1991]

rebased, changed compareSetTTL to use lua scripts and added tests for compareSetTTL

[skjnldsv]

/backport to stable28

[skjnldsv]

/backport to stable27

[backportbot-nextcloud]

The backport to stable27 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable27
git pull origin stable27

# Create the new backport branch
git checkout -b fix/foo-stable27

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable27

Error: Unknown error

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport