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

fix #716 (Add partial content with range) #1634

Merged
merged 3 commits into from Nov 27, 2016

Conversation

Projects
None yet
3 participants
@WebFreak001
Contributor

WebFreak001 commented Nov 26, 2016

Mostly based on #716 (comment)

Changes to that comment: added -[X] format and sending partialContent status code. Also some cleaner code and checks against bad requests.

Not sure how to add an automatic test but I did manually test it locally using this very simple file server in chrome and firefox:

import vibe.http.server;
import vibe.http.fileserver;

shared static this()
{
	auto settings = new HTTPServerSettings;
	settings.port = 10716;
	listenHTTP(settings, serveStaticFiles("data"));
}

My aim was only fixing media streams (audio & video + seeking), but pausing downloads and resuming also works now.

@WebFreak001

This comment has been minimized.

Show comment
Hide comment
@WebFreak001

WebFreak001 Nov 27, 2016

Contributor

Using this on my websites now and everything seems to work as expected and users don't complain about video playback anymore. (Asked them explicitly for issues)

Contributor

WebFreak001 commented Nov 27, 2016

Using this on my websites now and everything seems to work as expected and users don't complain about video playback anymore. (Asked them explicitly for issues)

Show outdated Hide outdated http/vibe/http/fileserver.d
auto range = (*prange).chompPrefix("bytes=");
auto s = range.split("-");
// https://tools.ietf.org/html/rfc7233
// Range can be in form "-\d", "\d-" or "\d-\d"

This comment has been minimized.

@s-ludwig

s-ludwig Nov 27, 2016

Member

The range can also have "/\d+" or "/*" as an additional suffix, and multiple ranges can be separated by comma. We should probably check that s.length == 2 and that range.canFind(','). Actually supporting this feature seems less important, though.

Edit: Misread the part with the slash suffix, that applies really only to the response.

@s-ludwig

s-ludwig Nov 27, 2016

Member

The range can also have "/\d+" or "/*" as an additional suffix, and multiple ranges can be separated by comma. We should probably check that s.length == 2 and that range.canFind(','). Actually supporting this feature seems less important, though.

Edit: Misread the part with the slash suffix, that applies really only to the response.

This comment has been minimized.

@s-ludwig

s-ludwig Nov 27, 2016

Member

"" - not sure if that's just my browser, but that's supposed to be a backslash.

@s-ludwig

s-ludwig Nov 27, 2016

Member

"" - not sure if that's just my browser, but that's supposed to be a backslash.

This comment has been minimized.

@WebFreak001

WebFreak001 Nov 27, 2016

Contributor

I don't really know how to send the response back to the user when there is a comma in the request. Like imagine you request the first and last character, should the response be both characters combined together or should it be different?

@WebFreak001

WebFreak001 Nov 27, 2016

Contributor

I don't really know how to send the response back to the user when there is a comma in the request. Like imagine you request the first and last character, should the response be both characters combined together or should it be different?

This comment has been minimized.

@s-ludwig

s-ludwig Nov 27, 2016

Member

It needs to be sent using a multi-part response. But I'd just send a 400 Bad Request back instead, as I don't think that any typical client will ever request multiple ranges.

@s-ludwig

s-ludwig Nov 27, 2016

Member

It needs to be sent using a multi-part response. But I'd just send a 400 Bad Request back instead, as I don't think that any typical client will ever request multiple ranges.

This comment has been minimized.

@s-ludwig

s-ludwig Nov 27, 2016

Member

...or rather 501 Not Implemented

@s-ludwig

s-ludwig Nov 27, 2016

Member

...or rather 501 Not Implemented

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Nov 27, 2016

Member

Instead of res.bodyWriter.write(new LimitedInputStream(fil, rangeEnd - rangeStart + 1));, write accepts a length parameter: res.bodyWriter.write(fil, rangeEnd - rangeStart + 1);. The writeRawBody optimization (enables the use of sendfile) could also be used for the range, but I'm afraid that this will uncover a bug in the current implementation (I think it doesn't check File.tell before calling sendfile).

The functionality itself looks good AFAICS.

Member

s-ludwig commented Nov 27, 2016

Instead of res.bodyWriter.write(new LimitedInputStream(fil, rangeEnd - rangeStart + 1));, write accepts a length parameter: res.bodyWriter.write(fil, rangeEnd - rangeStart + 1);. The writeRawBody optimization (enables the use of sendfile) could also be used for the range, but I'm afraid that this will uncover a bug in the current implementation (I think it doesn't check File.tell before calling sendfile).

The functionality itself looks good AFAICS.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Nov 27, 2016

Member

Okay, looks good to merge. Thanks for tackling this!

I'll look into adding a test after the merge.

Member

s-ludwig commented Nov 27, 2016

Okay, looks good to merge. Thanks for tackling this!

I'll look into adding a test after the merge.

@s-ludwig s-ludwig merged commit 6805267 into vibe-d:master Nov 27, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
if (s[0].length) {
rangeStart = s[0].to!ulong;
rangeEnd = s[1].length ? s[1].to!ulong : dirent.size;
} else if (s[1].length) {

This comment has been minimized.

@yazd

yazd Nov 27, 2016

Contributor

You need to check length of s before you access s[1].
The client can send malformed headers.

@yazd

yazd Nov 27, 2016

Contributor

You need to check length of s before you access s[1].
The client can send malformed headers.

This comment has been minimized.

@WebFreak001

WebFreak001 Nov 27, 2016

Contributor

Uh well this PR is merged already and I recreated the repo so I can't edit this anymore, I will make a separate PR.

@WebFreak001

WebFreak001 Nov 27, 2016

Contributor

Uh well this PR is merged already and I recreated the repo so I can't edit this anymore, I will make a separate PR.

s-ludwig added a commit that referenced this pull request Dec 19, 2016

Merge pull request #1634 from WebFreak001/master
fix #716 (Add partial content with range)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment