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

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

Merged
merged 3 commits into from
Nov 27, 2016
Merged

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

merged 3 commits into from
Nov 27, 2016

Conversation

WebFreak001
Copy link
Contributor

@WebFreak001 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
Copy link
Contributor Author

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)

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"
Copy link
Member

@s-ludwig s-ludwig Nov 27, 2016

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

...or rather 501 Not Implemented

@s-ludwig
Copy link
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.

@s-ludwig
Copy link
Member

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
if (s[0].length) {
rangeStart = s[0].to!ulong;
rangeEnd = s[1].length ? s[1].to!ulong : dirent.size;
} else if (s[1].length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants