Skip to content

properly lock the target file on dav upload when not using part files #9473

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
Merged
MorrisJobke merged 1 commit into from
Jun 11, 2018
Merged

properly lock the target file on dav upload when not using part files #9473

MorrisJobke merged 1 commit into from
Jun 11, 2018

Conversation

@icewind1991
Copy link
Member

@icewind1991 icewind1991 commented May 14, 2018

While non-partfile storages don't have the file corruption problem on current read/writes (which is why I left this until now) not locking the target file during write can still lead to an inconsistent filecache state because the scanner is not being blocked.

@icewind1991 icewind1991 added the 3. to review Waiting for reviews label May 14, 2018
@icewind1991 icewind1991 added this to the Nextcloud 14 milestone May 14, 2018
rullzer
rullzer reviewed May 14, 2018
View reviewed changes
apps/dav/lib/Connector/Sabre/File.php Outdated
throw new FileLocked($e->getMessage(), $e->getCode(), $e);
}
throw new FileLocked($e->getMessage(), $e->getCode(), $e);
}
Copy link
Member

@rullzer rullzer May 14, 2018

Choose a reason for hiding this comment

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

No need for two ifs ;)

Copy link
Member

@MorrisJobke MorrisJobke May 15, 2018

Choose a reason for hiding this comment

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

Why? IMO it makes sense to split it. 🤔

Copy link
Member

@rullzer rullzer May 15, 2018

Choose a reason for hiding this comment

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

I meant the if at

https://github.com/nextcloud/server/pull/9473/files#diff-be504dc886734275f06114e4306fa215R203
and
https://github.com/nextcloud/server/pull/9473/files#diff-be504dc886734275f06114e4306fa215R217

The try catch need to be split sure ;)

Copy link
Member Author

@icewind1991 icewind1991 May 15, 2018

Choose a reason for hiding this comment

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

fixed

@icewind1991 icewind1991 force-pushed the dav-upload-no-partfile-lock branch from 8b854ee to 891460a Compare May 15, 2018 12:00
@codecov
Copy link

codecov bot commented May 15, 2018 •
edited
Loading

Codecov Report

Merging #9473 into master will decrease coverage by 20.65%.
The diff coverage is 0%.

@@              Coverage Diff              @@
##             master    #9473       +/-   ##
=============================================
- Coverage     51.92%   31.27%   -20.66%     
+ Complexity    25769    25746       -23     
=============================================
  Files          1635     1641        +6     
  Lines         95434    96518     +1084     
  Branches       1308     1393       +85     
=============================================
- Hits          49557    30182    -19375     
- Misses        45877    66336    +20459
Impacted Files Coverage Δ Complexity Δ
apps/dav/lib/Connector/Sabre/File.php 1.5% <0%> (-71.29%) 99 <0> (-1)
apps/files_trashbin/lib/Command/Expire.php 0% <0%> (-100%) 3% <0%> (ø)
apps/dav/lib/Connector/Sabre/Server.php 0% <0%> (-100%) 1% <0%> (ø)
apps/user_ldap/lib/Migration/UUIDFixUser.php 0% <0%> (-100%) 1% <0%> (ø)
...yBuilder/FunctionBuilder/SqliteFunctionBuilder.php 0% <0%> (-100%) 1% <0%> (ø)
lib/private/Files/Mount/CacheMountProvider.php 0% <0%> (-100%) 4% <0%> (ø)
...ivate/Files/Cache/Wrapper/CachePermissionsMask.php 0% <0%> (-100%) 3% <0%> (ø)
lib/private/DB/SQLiteSessionInit.php 0% <0%> (-100%) 4% <0%> (ø)
lib/private/Files/Cache/Wrapper/JailPropagator.php 0% <0%> (-100%) 1% <0%> (ø)
...s/dav/lib/Connector/Sabre/Exception/FileLocked.php 0% <0%> (-100%) 3% <0%> (ø)
... and 450 more

rullzer
rullzer approved these changes May 15, 2018
View reviewed changes
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

Makes sense!

@MorrisJobke
Copy link
Member

MorrisJobke commented May 16, 2018

Unit tests fail. :/

rullzer
rullzer reviewed May 24, 2018
View reviewed changes
apps/dav/lib/Connector/Sabre/File.php Outdated
$partStorage->unlink($internalPartPath);
if ($needsPartFile) {
if ($view) {
$this->emitPreHooks($exists);
Copy link
Member

@rullzer rullzer May 24, 2018

Choose a reason for hiding this comment

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

You ignore the result here now. So if the prehook returns false you should probably also fail right?

Copy link
Member

@MorrisJobke MorrisJobke Jun 11, 2018

Choose a reason for hiding this comment

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

See the unit tests, which fail now due to this.

MorrisJobke
MorrisJobke requested changes Jun 11, 2018
View reviewed changes
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

See unit tests and @rullzer's comment.

@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jun 11, 2018
@icewind1991
properly lock the target file on dav upload when not using part files
f017f43 
Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991 icewind1991 force-pushed the dav-upload-no-partfile-lock branch from 891460a to f017f43 Compare June 11, 2018 13:27
@icewind1991
Copy link
Member Author

icewind1991 commented Jun 11, 2018

Fixed the unit test

@MorrisJobke MorrisJobke merged commit 61fa284 into master Jun 11, 2018
@MorrisJobke MorrisJobke deleted the dav-upload-no-partfile-lock branch June 11, 2018 14:54
@MorrisJobke MorrisJobke added 3. to review Waiting for reviews backport-request and removed 2. developing Work in progress labels Jun 11, 2018
@MorrisJobke
Copy link
Member

MorrisJobke commented Jun 11, 2018

@icewind1991 Does a backport to 13 makes sense or should we skip it?

@icewind1991
Copy link
Member Author

icewind1991 commented Jun 11, 2018

backport makes sense imo

@MorrisJobke
Copy link
Member

MorrisJobke commented Jun 12, 2018

@icewind1991 Could you prepare the PR?

@icewind1991 icewind1991 mentioned this pull request Jun 25, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rullzer rullzer rullzer approved these changes

@MorrisJobke MorrisJobke MorrisJobke approved these changes

Assignees

No one assigned

Labels

3. to review Waiting for reviews

Projects

None yet

Milestone

Nextcloud 14

Development

Successfully merging this pull request may close these issues.

4 participants

@icewind1991 @MorrisJobke @rullzer