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.

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

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

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
745ee0f
fix #716
Nov 26, 2016
d3971b8
Using write overload instead of LimitedInputStream
Nov 27, 2016
85e6ea1
Not implemented exception for multiple ranges
Nov 27, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 46 additions & 5 deletions http/vibe/http/fileserver.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import vibe.http.server;
import vibe.inet.message;
import vibe.inet.mimetypes;
import vibe.inet.url;
import vibe.stream.counting;

import std.conv;
import std.datetime;
Expand Down Expand Up @@ -311,7 +312,41 @@ private void sendFileImpl(scope HTTPServerRequest req, scope HTTPServerResponse
if ("Content-Encoding" in res.headers && isCompressedFormat(mimetype))
res.headers.remove("Content-Encoding");
res.headers["Content-Type"] = mimetype;
res.headers["Content-Length"] = to!string(dirent.size);
res.headers.addField("Accept-Ranges", "bytes");
ulong rangeStart = 0;
ulong rangeEnd = 0;
auto prange = "Range" in req.headers;

if (prange) {
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 •
edited
Loading

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

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.

rangeEnd = dirent.size;
auto len = s[1].to!ulong;
if (len >= rangeEnd)
rangeStart = 0;
else
rangeStart = rangeEnd - len;
} else {
throw new HTTPStatusException(HTTPStatus.badRequest);
}
if (rangeEnd > dirent.size)
rangeEnd = dirent.size;
if (rangeStart > rangeEnd)
rangeStart = rangeEnd;
if (rangeEnd)
rangeEnd--; // End is inclusive, so one less than length
// potential integer overflow with rangeEnd - rangeStart == size_t.max is intended. This only happens with empty files, the + 1 will then put it back to 0
res.headers["Content-Length"] = to!string(rangeEnd - rangeStart + 1);
res.headers["Content-Range"] = "bytes %s-%s/%s".format(rangeStart < rangeEnd ? rangeStart : rangeEnd, rangeEnd, dirent.size);
res.statusCode = HTTPStatus.partialContent;
} else
res.headers["Content-Length"] = dirent.size.to!string;

// check for already encoded file if configured
string encodedFilepath;
Expand Down Expand Up @@ -366,8 +401,14 @@ private void sendFileImpl(scope HTTPServerRequest req, scope HTTPServerResponse
}
scope(exit) fil.close();

if (pce && !encodedFilepath.length)
res.bodyWriter.write(fil);
else res.writeRawBody(fil);
logTrace("sent file %d, %s!", fil.size, res.headers["Content-Type"]);
if (prange) {
fil.seek(rangeStart);
res.bodyWriter.write(new LimitedInputStream(fil, rangeEnd - rangeStart + 1));
logTrace("partially sent file %d-%d, %s!", rangeStart, rangeEnd, res.headers["Content-Type"]);
} else {
if (pce && !encodedFilepath.length)
res.bodyWriter.write(fil);
else res.writeRawBody(fil);
logTrace("sent file %d, %s!", fil.size, res.headers["Content-Type"]);
}
}