search

iron / staticfile

Static file-serving middleware for the Iron web framework.
MIT License
63 stars 56 forks source link

Set an ETag along Last-Modified. #92

Closed stephank closed 7 years ago

stephank commented 7 years ago

Quick stab at this, curious to hear what you think! (Good idea, bad idea?) This particular behavior roughly matches Node.js / Express, generating for example: W/d-588658dd.0

stephank commented 7 years ago

Unrelated CI failure, but I fixed it anyway! :)

Hoverbear commented 7 years ago

Hi! Thanks for this pull request! Sorry it took some time to respond.

I don't see any issues this pull request.

Could you maybe add a brief "Why" to the PR description or in a comment?

stephank commented 7 years ago

That's actually a very good question, which I hadn't even thought of. I went off implementing this for completeness, more than a real practical reason.

From what I gather, ETag can be strong or weak, and Last-Modified is always weak. Strong would indicate contents are exactly the same, and would probably involve (expensive) hashing for us. Weak just means clients can do cache validation, and the content is only semantically the same. (http://stackoverflow.com/a/1560098/224129)

On top of Last-Modified, the ETag header value in this PR adds filesize as a factor, which is a small benefit I suppose. Besides that, I'm not sure if there are (proxy) clients out there that'd draw additional benefit from the presence of ETag on top of Last-Modified.

I rebased on master.

Hoverbear commented 7 years ago

Are you happy with with this is for now, or would you like to look into Strong ETags as well?

stephank commented 7 years ago

Thanks, I'm happy with this as is! :)

Fwiw, strong ETags seem difficult to implement, and would probably have to be an option because of the performance implications. You'd have to buffer the entire response to hash it on the fly and still send the header, or cache the hash somehow and put some trust in your user.

Seems like something that could use an actual use case to offer some more insight before implementing. I don't have one. :)

Hoverbear commented 7 years ago

Great. Okay. :) I'm going to merge this. Thanks!