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

Implement range aware static resource handler #175

Merged
merged 3 commits into from Sep 11, 2015

Conversation

@pmlopes
Copy link
Member

commented Aug 20, 2015

Signed-off-by: Paulo Lopes paulo@mlopes.net

Implements: #68

Signed-off-by: Paulo Lopes <paulo@mlopes.net>
@pmlopes pmlopes added the to review label Aug 20, 2015
// parse it
Long offset = null;
// end byte is length - 1
Long end = fileProps.size() - 1;

This comment has been minimized.

Copy link
@purplefox

purplefox Aug 20, 2015

Contributor

fileProps.size() is evaluated multiple times in the method. Better to evaluate it once.

MultiMap headers = request.response().headers();
headers.set("Accept-Ranges", "bytes");
// send the content length even for HEAD requests
headers.set("Content-Length", Long.toString(end + 1 - (offset == null ? 0 : offset)));

This comment has been minimized.

Copy link
@purplefox

purplefox Aug 20, 2015

Contributor

Isn't the content-length header set automatically in the sendfile method in core?

This comment has been minimized.

Copy link
@pmlopes

pmlopes Aug 21, 2015

Author Member

Core will only set the header if the data is really sent, for a HEAD request since there is no data being sent the content-length is always 0, while is should be the length of the file.

I could move it to the if statement bellow, however core does check if the header is already set and only if not set will calculate it and set.

writeCacheHeaders(request, fileProps);

// notify client we support range requests
MultiMap headers = request.response().headers();
headers.set("Accept-Ranges", "bytes");

This comment has been minimized.

Copy link
@purplefox

purplefox Aug 20, 2015

Contributor

Do we really want to set this header for all responses? Maybe it should be configurable?

This comment has been minimized.

Copy link
@pmlopes

pmlopes Aug 21, 2015

Author Member

Actually we do, if we don't send it browsers will not, for example, resume downloads or media players will not seek to a specific byte to start streaming form.

If we make it configurable then the whole resumable feature is disable. That is also a option but i don't see why would one not want that for static files. Their content is not dynamic so it does not have any side effects and can help reduce bandwidth for bad connections that break often (think mobile for example).

Also this is only being added to static files, the header will not be sent to dynamic directory listings.

This comment has been minimized.

Copy link
@purplefox

purplefox Aug 24, 2015

Contributor

The issue is related to performance. No doubt people will benchmark vertx-web e.g. in the techempower benchmarks, and adding just a single extra header can kill performance compared to other frameworks.

}
}
} catch (NumberFormatException | IndexOutOfBoundsException e) {
context.fail(416);

This comment has been minimized.

Copy link
@vietj

vietj Sep 10, 2015

Contributor

rather use a constant here than 416.

This comment has been minimized.

Copy link
@pmlopes

pmlopes Sep 10, 2015

Author Member

humm, all other http status are expressed by their numeral, 200, 403, 401, 206... why should the range not satisfiable be a constant? Or are you suggesting to replace all numerals with constants?

This comment has been minimized.

Copy link
@vietj

vietj Sep 10, 2015

Contributor

yes, something like public static final int FOO = 416

pmlopes added a commit that referenced this pull request Sep 11, 2015
Implement range aware static resource handler
@pmlopes pmlopes merged commit c02e452 into master Sep 11, 2015
@pmlopes pmlopes removed the to review label Sep 11, 2015
@pmlopes pmlopes deleted the feature/range-requests branch Sep 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.