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.

Sign up for GitHub

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

Jump to bottom

Lock upload during finalization #9500

Merged
merged 3 commits into from
Mar 9, 2022
Merged

Lock upload during finalization #9500

merged 3 commits into from
Mar 9, 2022

Conversation

TheOneRing
Copy link
Contributor

@TheOneRing TheOneRing commented Mar 9, 2022 •
edited
Loading

Client Server State
A A Same version on both sides
B A Versions differ, client starts upload
B A During the upload, B gets locked
B B B is uploaded, Local B is locked so we don't enter the state to the db
C B The local db was not updated, we don't know about B on the server, local file gets updated and becomes C
C B We have C locally and B on the server -> clash

@TheOneRing TheOneRing requested a review from a team March 9, 2022 10:40
@TheOneRing TheOneRing force-pushed the work/fix_upload_locked branch 2 times, most recently from 9c8fce7 to 8b479d5 Compare March 9, 2022 11:50
dragotin
dragotin reviewed Mar 9, 2022
View reviewed changes
Copy link
Contributor

@dragotin dragotin left a comment

Choose a reason for hiding this comment

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

This patch means that any call to isFileLocked() creates a lock on the file, which is released automatically. That function is called often, are you sure there are no side effects?

src/common/utility_win.cpp
@@ -319,4 +319,31 @@ Utility::NtfsPermissionLookupRAII::~NtfsPermissionLookupRAII()
qt_ntfs_permission_lookup--;
}


Utility::Handle::Handle(HANDLE h, std::function<void(HANDLE)> &&close)
: _handle(h)
Copy link
Contributor

Choose a reason for hiding this comment

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

is the close function used anywhere so far? If not I'd rather not have it. You're only using the default, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the vfs plugin, the handle class is just moved to the main repo.

@TheOneRing
Copy link
Contributor Author

This patch means that any call to isFileLocked() creates a lock on the file, which is released automatically. That function is called often, are you sure there are no side effects?

We did the same before, just explicitly called CloseHandle before.

@TheOneRing TheOneRing requested a review from a team March 9, 2022 12:35
dragotin
dragotin approved these changes Mar 9, 2022
View reviewed changes
Copy link
Contributor

@dragotin dragotin left a comment

Choose a reason for hiding this comment

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

ok

@TheOneRing TheOneRing added this to the 2.10.1 milestone Mar 9, 2022
dschmidt
dschmidt approved these changes Mar 9, 2022
View reviewed changes
Copy link
Member

@dschmidt dschmidt left a comment •
edited
Loading

Choose a reason for hiding this comment

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

looks reasonable ... but I'm not familiar with this part of the code base

changelog/unreleased/enterprise-5052 Outdated Show resolved Hide resolved
@TheOneRing TheOneRing force-pushed the work/fix_upload_locked branch from 488be63 to 29c5b4e Compare March 9, 2022 12:57
@TheOneRing TheOneRing merged commit 2d3d672 into 2.10 Mar 9, 2022
@delete-merged-branch delete-merged-branch bot deleted the work/fix_upload_locked branch March 9, 2022 12:58
@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 9, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@TheOneRing TheOneRing mentioned this pull request Mar 10, 2022
Closed
@gabi18 gabi18 mentioned this pull request Mar 15, 2022
Closed
55 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dschmidt dschmidt dschmidt approved these changes

@dragotin dragotin dragotin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.10.1
Development

Successfully merging this pull request may close these issues.
3 participants
@TheOneRing @dschmidt @dragotin