Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update the file cache within the write lock #17017

Merged
merged 4 commits into from
Jun 18, 2015
Merged

Conversation

icewind1991
Copy link
Contributor

Prevents any other process from updating the cache while we're working on it.

Fixes #13391

With just this PR trying to list the folder being renamed will result in a locked exception being thrown and the sync client will try again later, together with #16963 you can still browser the folder being renamed (actually trying to download the file will result in a locked error) and the sync client wont detect any changes until the rename is done.

cc @PVince81 @DeepDiver1975

@@ -124,13 +124,17 @@ public function getData($path) {
* @param int $reuseExisting
* @param int $parentId
* @param array | null $cacheData existing data in the cache for the file to be scanned
* @param bool $lock
Copy link
Contributor

Choose a reason for hiding this comment

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

false to disable cache locking ?

@PVince81
Copy link
Contributor

This PR alone doesn't fully fix #13391, but in my tests the folder contents was not completely removed. However, after a few tries a few files went missing.

Now testing together with #16963 as advised in the original comment.

@PVince81
Copy link
Contributor

Ran the rename test case 10 times with this PR merged together with #16963 and the data loss seems to have disappeared!
I even had a case where I renamed twice before the sync client could catch up, still worked fine.

@icewind1991 you rock 😄

How about just merging into this PR here to make it easier for regression testing ?

I think we cannot merge this yet because there is still this regression #16963 (comment) (but I'm ok if we do merge and fix it separately)

@scrutinizer-notifier
Copy link

A new inspection was created.

@PVince81
Copy link
Contributor

👍 (please fix the comments)

@icewind1991
Copy link
Contributor Author

(please fix the comments)

Already done 😄

@icewind1991
Copy link
Contributor Author

@DeepDiver1975 @MorrisJobke please review

@ghost
Copy link

ghost commented Jun 18, 2015

🚀 Test PASSed.🚀
chuck

@PVince81
Copy link
Contributor

Tested both PRs merged, fixes #11795 too

@PVince81
Copy link
Contributor

Tested both PRs merged, fixes #16584 too

@nickvergessen
Copy link
Contributor

👍 tested together with #16963 (comment)

PVince81 pushed a commit that referenced this pull request Jun 18, 2015
update the file cache within the write lock
@PVince81 PVince81 merged commit 0b34d88 into master Jun 18, 2015
@PVince81 PVince81 deleted the cache-update-write-lock branch June 18, 2015 16:27
@MorrisJobke MorrisJobke added this to the 8.1-current milestone Jun 18, 2015
@lock lock bot locked as resolved and limited conversation to collaborators Aug 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data loss on rename of a 49 GB folder
5 participants