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

Add cache control headers to http static file handler + a few more mi… #2470

Merged
merged 5 commits into from
Jan 18, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
31 changes: 29 additions & 2 deletions spec/std/http/server/handlers/static_file_handler_spec.cr
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
require "spec"
require "http/server"

private def handle(request, fallthrough = true, directory_listing = true)
private def handle(request, fallthrough = true, directory_listing = true, ignore_body = false)
io = IO::Memory.new
response = HTTP::Server::Response.new(io)
context = HTTP::Server::Context.new(request, response)
handler = HTTP::StaticFileHandler.new "#{__DIR__}/static", fallthrough, directory_listing
handler.call context
response.close
io.rewind
HTTP::Client::Response.from_io(io)
HTTP::Client::Response.from_io(io, ignore_body)
end

describe HTTP::StaticFileHandler do
Expand All @@ -21,6 +21,33 @@ describe HTTP::StaticFileHandler do
response.body.should eq(File.read("#{__DIR__}/static/test.txt"))
end

context "with header If-Modified-Since" do
it "should return 304 Not Modified if file mtime is equal" do
headers = HTTP::Headers.new
headers["If-Modified-Since"] = HTTP.rfc1123_date(File.stat("#{__DIR__}/static/test.txt").mtime)
response = handle HTTP::Request.new("GET", "/test.txt", headers), ignore_body: true
response.status_code.should eq(304)
response.headers["Last-Modified"].should eq(HTTP.rfc1123_date(File.stat("#{__DIR__}/static/test.txt").mtime))
end

it "should return 304 Not Modified if file mtime is older" do
headers = HTTP::Headers.new
headers["If-Modified-Since"] = HTTP.rfc1123_date(File.stat("#{__DIR__}/static/test.txt").mtime + 1.hour)
response = handle HTTP::Request.new("GET", "/test.txt", headers), ignore_body: true
response.status_code.should eq(304)
response.headers["Last-Modified"].should eq(HTTP.rfc1123_date(File.stat("#{__DIR__}/static/test.txt").mtime))
end

it "should serve file if file mtime is younger" do
headers = HTTP::Headers.new
headers["If-Modified-Since"] = HTTP.rfc1123_date(File.stat("#{__DIR__}/static/test.txt").mtime - 1.hour)
response = handle HTTP::Request.new("GET", "/test.txt")
response.status_code.should eq(200)
response.headers["Last-Modified"].should eq(HTTP.rfc1123_date(File.stat("#{__DIR__}/static/test.txt").mtime))
response.body.should eq(File.read("#{__DIR__}/static/test.txt"))
end
end

it "should list directory's entries" do
response = handle HTTP::Request.new("GET", "/")
response.status_code.should eq(200)
Expand Down
17 changes: 17 additions & 0 deletions src/http/server/handlers/static_file_handler.cr
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,23 @@ class HTTP::StaticFileHandler
context.response.content_type = "text/html"
directory_listing(context.response, request_path, file_path)
elsif is_file
last_modified = File.stat(file_path).mtime
Copy link
Contributor

Choose a reason for hiding this comment

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

Argh, more stat usages to fix in #5584! :)

context.response.headers["Last-Modified"] = HTTP.rfc1123_date(last_modified)

if if_modified_since = context.request.headers["If-Modified-Since"]?
# TODO: Use a more generalized time format parser for better compatibility to RFC 7232
header_time = Time.parse(if_modified_since, "%a, %d %b %Y %H:%M:%S GMT")

# File mtime probably has a higher resolution than the header value.
# An exact comparison might be slightly off, so we add 1s padding.
# Static files should generally not be modified in subsecond intervals, so this is perfectly safe.
# This might replaced by a more sophisticated time comparison when it becomes available.
if last_modified <= header_time + 1.second
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can use last_modified.total_seconds.to_i == header_time.total_seconds.to_i?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this could lead to failing edge cases because of rounding errors.

Copy link
Member

Choose a reason for hiding this comment

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

This would certainly not have the expected results because a time at 0.999_999_999 s will be just one nanosecond away from 1.0s but total_seconds != total_seconds.

Copy link
Member

Choose a reason for hiding this comment

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

While it's not super critical here, it doesn't make sense to sacrifice precision for performance since the whole point is to improve efficiency if the time stamp is pretty recent.

context.response.status_code = 304
return
end
end

context.response.content_type = mime_type(file_path)
context.response.content_length = File.size(file_path)
File.open(file_path) do |file|
Expand Down