Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#14560 closed (fixed)

UpdateCacheMiddleware does not save responses for HEAD requests

Reported by: codemonkey Owned by: nobody
Component: Core (Cache system) Version: master
Severity: Keywords: cache HEAD middleware
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:


If you are sending HEAD request to your application, response won't be cached.
But if you are sending GET request first, response will be cached and following HEAD request will be answered directly from cache.
According to the comment in UpdateCacheMiddleware's process_response() method, this behaviour depends on inexistent HTTPMiddleware. Due to the fact that response content for HEAD requests is throwed away by response fixers after all middlewares are applied, it could be safely dropped.

Patch that fixes caching of HEAD requests is attached to this ticket.

Attachments (1)

fix_head_cache.diff (6.7 KB) - added by codemonkey 5 years ago.
Patch that fixes caching of HEAD requests, provides tests and docs.

Download all attachments as: .zip

Change History (7)

comment:1 Changed 5 years ago by Honza_Kral

  • Keywords middleware added
  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

If we want to cache HEAD requests, we need to store it as a separate cache key since view can (with reasonable use-cases) render different body output for HEAD and GET requests. It is probably OK to storing GET result in cache and use that even for HEAD requests (especially since that is current behavior), but we have to avoid it the other way around.

So if the request being stored in cache is HEAD, store it under different key and when looking for HEAD request in FetchFromCacheMiddleware , look at both keys (either using get_many() or two cache requests, depending on what's faster for memcached).

comment:2 Changed 5 years ago by codemonkey

While writing patch I was guided by that comment. I don't think it is really necessary to split caching logic for GET and HEAD requests because response body for HEAD requests are always throwing away in WSGIHandler. If it is still necessary I could provide better patch.

comment:3 Changed 5 years ago by Honza_Kral

Oh, I didn't see the comments, but nonetheless - it's not mentioned in the docs so we cannot rely on that. I am not worried about using a GET response to answer a HEAD request, that should work (for the reasons you mention) - I am worried about people optimizing their view code (skipping expensive template rendering for HEAD requests) and the middleware using HEAD responses to answer GET request.

The ideal patch would:

  • change this comment to say that GET and HEAD responses' headers must be the same
  • add this to the docs
  • provide tests
  • do all of the above

Changed 5 years ago by codemonkey

Patch that fixes caching of HEAD requests, provides tests and docs.

comment:4 Changed 5 years ago by codemonkey

  • Needs tests unset
  • Patch needs improvement unset

comment:5 Changed 5 years ago by Honza_Kral

  • Resolution set to fixed
  • Status changed from new to closed

Fixed in [14391]

comment:6 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

Note: See TracTickets for help on using tickets.
Back to Top