add support for Last-Modified and If-Modified-Since headers #1376
Conversation
There was a problem hiding this comment.
Generally speaking, I'm in favor of having this or at least something similar. I have not audited the date comparison code, but that will really need to be correct. I do have two wider concerns:
- Reliability of timestamps from the filesystem is not necessarily great; there are many pitfalls here.
StaticFiles/NamedFileseems like an okay place to do this with a few safeguards, such as ignoring filesystem dates that are from the future. - Interactions with other preconditions. This PR seems to ignore most of https://tools.ietf.org/html/rfc7232#section-6. That might be okay! For example, maybe NamedFile specifically does not ever have to worry about whether
If-None-Matchis present, sinceNamedFilenever generates anETagheader. But like the date comparisons, this is an easy thing to get subtly wrong and needs some scrutiny. - And one very minor downside / bikeshed: with this implementation, we would effectively remove "File with extension-based MIME detection" and replace it with "File with extension-based MIME detection and filesystem-based time checking"; maybe this should be a builder or a separate type so that the two features aren't tied together.
| impl<'r> Responder<'r, 'static> for NamedFile { | ||
| fn respond_to(self, req: &'r Request<'_>) -> response::Result<'static> { | ||
| // using the file handle `self.1` is too much trouble here, tokio's `File::metadata` method | ||
| // is async, so is into_std, and try_into_std gives you a `io::Result<std::fs::File>` |
There was a problem hiding this comment.
We can't safely use std::fs::metadata even in this function, since it's blocking. The problem of respond_to being async or non-async came up a bit, and we had resolved it as "async work should happen in an async constructor" (in this case NamedFile::open).
This does mean that we would need to query the modification time even if there is no If-Modified-Since header to compare to; I'm not sure how big of a problem that is in terms of performance.
| .map(|duration| OffsetDateTime::from_unix_timestamp(duration.as_secs() as i64)); | ||
|
|
||
| // if file_mod_time is < this date, we don't need to re-send the file. | ||
| let cutoff = req.headers().get_one("If-Modified-Since") |
There was a problem hiding this comment.
This should ensure the method was GET or HEAD, and maybe should check other preconditions.
|
Checking in: what's the status here? We also have #1219; should one be closed in favor of another? |
|
I hadn't realized there were two PRs for this. I'm closing this one in favor of #1219, which is opt-in (resolving most of my concerns about reliability and composability) and more complete (including examples and tests). |
#989, #95
Adds support for Last-Modified and If-Modified-Since headers.
NamedFileis requested with the If-Modified-Since header set and the file was last modified before the date in the header, the server instead responds with 304 Not ModifiedI'm pretty new to rocket so let me know if I'm doing anything wrong and if the style is ok! (no rustfmt... shame 😛 )