Skip to content

Avoid locking on restore/delete and limit concurrent requests - #36465

Merged
juliusknorr merged 3 commits into
masterfrom
bugfix/trashbin-concurrency
Feb 15, 2023
Merged

Avoid locking on restore/delete and limit concurrent requests#36465
juliusknorr merged 3 commits into
masterfrom
bugfix/trashbin-concurrency

Conversation

@juliusknorr

@juliusknorrjuliusknorr commented Feb 1, 2023

Copy link
Copy Markdown
Member

Summary

This is a frontend safeguard to avoid running into backend file locks when deleting or restoring multiple files. Even after https://github.com/nextcloud/server/pull/28438/files#diff-5450a6734b54a8b5e2c2f7521098ba6d05993e8bbebffeee140e93ec349e4127R978 this could still happen if the retry timeout was exceeding 15 seconds.

Checklist

@juliusknorr
juliusknorr requested review from a team, Pytal, skjnldsv and szaimen and removed request for a teamFebruary 1, 2023 07:20
@juliusknorrjuliusknorr added bug 3. to review Waiting for reviews labels Feb 1, 2023
@szaimenszaimen added this to the Nextcloud 26 milestone Feb 1, 2023

@szaimenszaimen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐘

@icewind1991

icewind1991 commented Feb 1, 2023

Copy link
Copy Markdown
Member

I think it's fine to change the scanning logic to only scan 'files_trashbin/versions/'. $filename instead of 'files_trashbin/versions/', which should help prevent conflicts

Edit: that doesn't work as they don't have a common parent but are instead named "$filename.vXXX.dXXX"

@icewind1991

Copy link
Copy Markdown
Member

Note that I'm not sure if the scan logic is still needed, I know that in the past we didn't actively maintain the cache outside of /$user/files but I believe that is no longer the case

@juliusknorr

Copy link
Copy Markdown
MemberAuthor

Note that I'm not sure if the scan logic is still needed, I know that in the past we didn't actively maintain the cache outside of /$user/files but I believe that is no longer the case

I had the same assumption in https://github.com/nextcloud/server/pull/28438/files#r691270467 so should we just drop it?

@juliusknorr

Copy link
Copy Markdown
MemberAuthor

Deleting and restoring versions seems to still work fine with that code part removed.

@blizzzblizzz mentioned this pull request Feb 1, 2023
@juliusknorr
juliusknorrforce-pushed the bugfix/trashbin-concurrency branch from 4f9d903 to 9ebe372CompareFebruary 1, 2023 20:33
@icewind1991

Copy link
Copy Markdown
Member

Do we want to increase the concurrency a bit if we remove the scan?

…ocking in the backend
Signed-off-by: Julius Härtl <jus@bitgrid.net>
…rsions from trash
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
juliusknorrforce-pushed the bugfix/trashbin-concurrency branch from 9ebe372 to 7f913deCompareFebruary 2, 2023 16:23
@juliusknorrjuliusknorr changed the title fix: Limit trashbin restore/delete to 2 concurrent request to avoid locking in the backendAvoid locking on restore/delete and limit concurrent requestsFeb 2, 2023
@juliusknorr

Copy link
Copy Markdown
MemberAuthor

Increased to 4 which seems a reasonable value for now.

@icewind1991

Copy link
Copy Markdown
Member

/compile

Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@juliusknorr
juliusknorr merged commit 63696fe into masterFeb 15, 2023
@juliusknorr
juliusknorr deleted the bugfix/trashbin-concurrency branch February 15, 2023 16:04
@blizzzblizzz mentioned this pull request Feb 15, 2023
@juliusknorr

Copy link
Copy Markdown
MemberAuthor

/backport 7f913de to stable25

@juliusknorr

Copy link
Copy Markdown
MemberAuthor

/backport 7f913de to stable24

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

Labels

3. to reviewWaiting for reviewsbug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@juliusknorr@icewind1991@skjnldsv@szaimen@nextcloud-command