Opened 5 years ago

Closed 5 years ago

#30812 closed Cleanup/optimization (fixed)

ConditionalGetMiddleware returns 304 if ETag is the same but Last-Modified has changed.

Reported by: Flavio Curella Owned by: Dart
Component: Core (Cache system) Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Mariusz Felisiak)

ConditionalGetMiddleware in combination with apache x-sendfile (django-sendfile) doesn't work properly.

Each response gets a ETag generated based on response.content which is an empty string in the case of a x-sendfile response, so each time the file is accessed, the ETag generated by ConditionalGetMiddleware is the same. Regardless of the changed file/changed mtime. In get_conditional_response() the ETag (which is always the same hash of empty string) is checked first and returns a 304 because it ignores Last-Modified time. Django shouldn't return 304 if ETag is the same but Last-Modified has changed.

Related with #29241.

Change History (11)

comment:1 by Mariusz Felisiak, 5 years ago

Description: modified (diff)
Summary: Return a 304 if ETag is the same but Last-Modified has changedConditionalGetMiddleware returns 304 if ETag is the same but Last-Modified has changed.
Triage Stage: UnreviewedAccepted
Version: 2.2master

Thanks for the ticket (please try to add more descriptive description in the future).

comment:2 by Dart, 5 years ago

Owner: changed from nobody to Dart
Status: newassigned

comment:3 by Dart, 5 years ago

Hello! Please, check my pull request. I dont know is it django "right" way, but its works. So my solution was to check x-sendfile header and if it exists ignore etag comparison, because in this situation etag does not matter.
https://github.com/django/django/pull/11878

comment:5 by Mariusz Felisiak, 5 years ago

Thanks for this patch, however I don't think that it is a right solution. This issue is not about nginx, it is about ConditionalGetMiddleware and how it behaves with the same Etag and changed Last-Modified time (see https://tools.ietf.org/html/rfc7232).

comment:6 by Claude Paroz, 5 years ago

One possible solution would be to not generate an Etag for empty content. I didn't find documentation about that specific case, but for me, it does not make much sense to cache (return 304) empty responses.

As for the precedence between Etag and Last modified conditions, I think Django is respecting the specs.

comment:7 by Dart, 5 years ago

So as i see there two posible solutions:

  • remove generation etag from middleware if empty content exists

or

  • add one more attributes for generation etag

No matter from rfc-7232:

An origin server SHOULD send an ETag for any selected representation
for which detection of changes can be reasonably and consistently
determined, since the entity-tag's use in conditional requests and
evaluating cache freshness ([RFC7234]) can result in a substantial
reduction of HTTP network traffic and can be a significant factor in
improving service scalability and reliability.

Or we can do nothing and rely on customer understanding

comment:8 by Dart, 5 years ago

https://github.com/django/django/pull/11891
Add checking response content before generating Etag.

comment:9 by Claude Paroz, 5 years ago

Has patch: set

comment:10 by Mariusz Felisiak, 5 years ago

Type: BugCleanup/optimization

comment:11 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In ee6b1718:

Fixed #30812 -- Made ConditionalGetMiddleware set ETag only for responses with non-empty content.

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