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

Lock file during download #17009

Merged
merged 2 commits into from
Jun 26, 2015
Merged

Lock file during download #17009

merged 2 commits into from
Jun 26, 2015

Conversation

PVince81
Copy link
Contributor

Put a shared lock on a file before download it, and free it at the end of the transfer.

My worries here:

  1. Shared lock during download will prevent other users to update/overwrite that file in the time being. Ideal would be able to deliver an older copy of the file
  2. What happens if the download takes too long and PHP times out, will the lock stay forever ?

@DeepDiver1975 @icewind1991 what do you think ?

A similar approach will be needed for zip files, see #16960 (comment)

This will throw a LockedException if a concurrent request is currently
touching that file.
@PVince81 PVince81 added this to the 8.1-current milestone Jun 18, 2015
@PVince81
Copy link
Contributor Author

This for the download error message part for here #16701

Another alternative would be to just try and lock the file before the upload, and then directly unlock before the transfer starts, or after the first packet. (hacky)

@PVince81
Copy link
Contributor Author

Looks like max execution time is set to 0 here https://github.com/owncloud/core/blob/master/lib/private/files.php#L151 but it's only for zip files, not regular downloads.

@DeepDiver1975
Copy link
Member

  1. Shared lock during download will prevent other users to update/overwrite that file in the time being. Ideal would be able to deliver an older copy of the file

well - this is what we want to have - right? Don't touch the file while I'm downloading it or I'll get crap.

  1. What happens if the download takes too long and PHP times out, will the lock stay forever ?

This is why we need a TTL on locks - as discussed right from the beginning

@DeepDiver1975
Copy link
Member

Looks like max execution time is set to 0 here https://github.com/owncloud/core/blob/master/lib/private/files.php#L151 but it's only for zip files, not regular downloads.

we might want to change this and have execution time of 0 for any kind of download.
(Note: this whole messy file will die in 8.2 due to webdav 🎉 )

@ghost
Copy link

ghost commented Jun 18, 2015

🚀 Test PASSed.🚀
chuck

} catch (\OCP\Lock\LockedException $ex) {
$l = \OC::$server->getL10N('core');
$hint = method_exists($ex, 'getHint') ? $ex->getHint() : '';
\OC_Template::printErrorPage($l->t('File is currently busy, please try again later'), $hint);
Copy link
Member

Choose a reason for hiding this comment

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

brrr - this file has to die 💀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We still need it for zip download... unless we can come up with a WebDAV endpoint for zip download.

Also see #16960 (comment), very hard to decide whether to move forward

Copy link
Member

Choose a reason for hiding this comment

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

We still need it for zip download... unless we can come up with a WebDAV endpoint for zip download.

we have to do this anyhow from my pov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Raised zip webdav endpoint here #17014

@PVince81
Copy link
Contributor Author

TTL discussion here: #16966

@icewind1991
Copy link
Contributor

Shouldn't the fopen already handle the locking here?

@PVince81
Copy link
Contributor Author

Yes, it does. But then it's too late and we already sent the 200 OK + headers.

We need to be able to return another HTTP code before fopen (readfile) happens.

@DeepDiver1975
Copy link
Member

Should we simply disable xsendfile as soon as locking is enabled?

@DeepDiver1975
Copy link
Member

Should we simply disable xsendfile as soon as locking is enabled?

or drop this feature completely?

@DeepDiver1975
Copy link
Member

or drop this feature completely?

especially thinking about all our wrapper setups I really question if there is a single valid scenario for xsendfile left.

@karlitschek
Copy link
Contributor

lets disable it for now if filelocking is active and discuss again for 8.2

@DeepDiver1975
Copy link
Member

lets disable it for now if filelocking is active and discuss again for 8.2

agreed - I'll add this to this pr ...

@scrutinizer-notifier
Copy link

A new inspection was created.

@ghost
Copy link

ghost commented Jun 22, 2015

💣 Test FAILed. 💣
nooo432

@PVince81
Copy link
Contributor Author

@owncloud-bot retest this please

@PVince81
Copy link
Contributor Author

  • disable xsendfile if locking is enabled

@ghost
Copy link

ghost commented Jun 23, 2015

💣 Test FAILed. 💣
nooo432

@DeepDiver1975
Copy link
Member

disable xsendfile if locking is enabled

@PVince81 c74c8ef

@DeepDiver1975
Copy link
Member

Jenkins fails on oracle - can be ignored 👍

@DeepDiver1975
Copy link
Member

@th3fallen @icewind1991 please review - thx

@icewind1991
Copy link
Contributor

👍 looks good

DeepDiver1975 added a commit that referenced this pull request Jun 26, 2015
@DeepDiver1975 DeepDiver1975 merged commit 796aae4 into master Jun 26, 2015
@DeepDiver1975 DeepDiver1975 deleted the lock-downloadfile branch June 26, 2015 11:04
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants