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

Enable fseek for files in S3 storage #20033

Merged
merged 4 commits into from Apr 6, 2020
Merged

Enable fseek for files in S3 storage #20033

merged 4 commits into from Apr 6, 2020

Conversation

icewind1991
Copy link
Member

Based on the work of @ahti in #19073

This allows nextcloud to use fseek when using a S3 storage backend, which enables range requests and thus video streaming including fast seeking, and other applications.

#8275 should be fixed by this.

The implementation is a new stream wrapper around the normal http stream. When fseek is used, a new stream is opened with a range header to begin at the right offset.

I'm not really a PHP developer, so there might be bugs, and there is no proper error handling when the storage backend doesn't reply with a content-range header (although I would expect S3 backends to support range requests properly), and probably in some other places, too.

I have not investigated this, but maybe this approach could also work for the other object store backends?

Adjusted to be generic of any http stream where the server supports range requests so it's easy to extend to other storage backends in the future

@icewind1991 icewind1991 added the 3. to review Waiting for reviews label Mar 19, 2020
@icewind1991 icewind1991 added this to the Nextcloud 19 milestone Mar 19, 2020
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

🐘

*/
private static function registerIfNeeded() {
if (!self::$registered) {
stream_wrapper_register(
Copy link
Contributor

Choose a reason for hiding this comment

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

stream_wrapper_register() will return FALSE if the protocol already has a handler.

https://www.php.net/manual/en/function.stream-wrapper-register.php

Do we really need a state (registered y/no)? I guess stream_wrapper_register will not register more than one handler for one protocol.

@rullzer
Copy link
Member

rullzer commented Apr 1, 2020

Needs a rebase

ahti and others added 3 commits April 1, 2020 15:21
Signed-off-by: Lukas Stabe <lukas@stabe.de>
Signed-off-by: Robin Appelman <robin@icewind.nl>
…return the wrong range

Signed-off-by: Robin Appelman <robin@icewind.nl>
lib/private/Files/Stream/SeekableHttpStream.php Outdated Show resolved Hide resolved
lib/private/Files/Stream/SeekableHttpStream.php Outdated Show resolved Hide resolved
lib/private/Files/Stream/SeekableHttpStream.php Outdated Show resolved Hide resolved
@rullzer
Copy link
Member

rullzer commented Apr 2, 2020

@icewind1991 please address the comments and then lets get this in

Signed-off-by: Robin Appelman <robin@icewind.nl>
@rullzer rullzer mentioned this pull request Apr 4, 2020
80 tasks
@rullzer rullzer merged commit f5919d5 into master Apr 6, 2020
@rullzer rullzer deleted the s3-seekable-stream branch April 6, 2020 19:38
@MorrisJobke
Copy link
Member

@rullzer @icewind1991 Do we want to backport this to 17 and 18?

@MorrisJobke
Copy link
Member

/backport to stable18

@MorrisJobke
Copy link
Member

/backport to stable17

paulijar added a commit to paulijar/server that referenced this pull request Sep 11, 2021
The PR nextcloud#20033 added support
for `fseek` for  the S3 storage backend. However, the seek mode SEEK_END
was left out that time. This PR fills this gap.

Signed-off-by: Pauli Järvinen <pauli.jarvinen@gmail.com>
backportbot-nextcloud bot pushed a commit that referenced this pull request Sep 20, 2021
The PR #20033 added support
for `fseek` for  the S3 storage backend. However, the seek mode SEEK_END
was left out that time. This PR fills this gap.

Signed-off-by: Pauli Järvinen <pauli.jarvinen@gmail.com>
backportbot-nextcloud bot pushed a commit that referenced this pull request Sep 20, 2021
The PR #20033 added support
for `fseek` for  the S3 storage backend. However, the seek mode SEEK_END
was left out that time. This PR fills this gap.

Signed-off-by: Pauli Järvinen <pauli.jarvinen@gmail.com>
backportbot-nextcloud bot pushed a commit that referenced this pull request Sep 20, 2021
The PR #20033 added support
for `fseek` for  the S3 storage backend. However, the seek mode SEEK_END
was left out that time. This PR fills this gap.

Signed-off-by: Pauli Järvinen <pauli.jarvinen@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants