Opened 17 years ago

Closed 8 years ago

Last modified 8 years ago

#5897 closed New feature (fixed)

Add Content-Length header in common middleware

Reported by: Scott Barr <scott@…> Owned by: ccahoon
Component: HTTP handling Version: dev
Severity: Normal Keywords: Content-Length middleware
Cc: clokep@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I've modified django.middleware.common.py to add a "Content-Length" header to the HTTP response if the response code is 200 and the "Content-Length" header hasn't already been supplied.

Attachments (1)

middleware.common.patch (515 bytes ) - added by Scott Barr <scott@…> 17 years ago.
Patch

Download all attachments as: .zip

Change History (16)

by Scott Barr <scott@…>, 17 years ago

Attachment: middleware.common.patch added

Patch

comment:1 by Simon Greenhill <dev@…>, 17 years ago

Triage Stage: UnreviewedReady for checkin

comment:2 by Adrian Holovaty, 16 years ago

Hmmm, what's the advantage of doing this? I don't necessarily like the overhead of doing a len() on every request...

comment:3 by Adrian Holovaty, 16 years ago

Triage Stage: Ready for checkinDesign decision needed

comment:4 by fumanchu, 16 years ago

The advantage is following the HTTP spec, which mandates that messages either:

  1. be length 0 by design (e.g. HEAD)
  2. use "chunked" encoding
  3. provide Content-Length
  4. use the multipart/byteranges media type, or
  5. close the connection.

Options 1, 2, and 4 are rare, so omitting C-L leaves us with (5) and therefore means conns can't ever use keepalive. I would say that any response with a buffered body, not just 200, should get a C-L response header.

In addition, the len(str) function seems to be nearly O(1) up to about 95MB:

>>> for size in range(10):
...     timeit.Timer("len(a)", "a = 'a' * %d" % (10 ** size)).timeit()
...     
0.090876075031882553
0.092112894236557996
0.091411897322145691
0.096701326565247825
0.095689815325690875
0.098474945838088412
0.09903919352878654
0.097267250446634979
0.09309835467915617
0.4417090719630572
>>> 10 ** 8
100000000
>>> _ / 1024
97656
>>> _ / 1024
95

comment:5 by ccahoon, 15 years ago

Owner: changed from nobody to ccahoon

comment:6 by ccahoon, 15 years ago

Three things:

That said, I am going to investigate further. This is closely related to #7581, which incidentally has a very similar bit of code to the patch provided here.

When dealing with streaming responses, probably intended for chunked transfer encoding, we want to ensure that we don't consume the whole generator/iterator in any effort to send a header that is not strictly required.

comment:7 by Graham King, 15 years ago

ConditionalGetMiddleware already does this (amongst other things). http://docs.djangoproject.com/en/dev/ref/middleware/#module-django.middleware.http
Is that not sufficient?

Also, Content-Length should be set for all header codes (Ajax and REST APIs will use a wider selection, 201 for example).
core/handlers/base.py will use http.conditional_content_removal to remove Content-Length for any response codes that shouldn't have it, so setting for all is safe.

comment:8 by Gabriel Hurley, 14 years ago

Severity: Normal
Type: New feature

comment:9 by Carl Meyer, 13 years ago

Easy pickings: unset
Resolution: wontfix
Status: newclosed
UI/UX: unset

Discussion with Alex at sprint: since this is not required by spec, and is already available in ConditionalGetMiddleware for those who want it, we don't want to go exhausting iterator responses by default in CommonMiddleware.

comment:10 by Aymeric Augustin, 10 years ago

Has patch: unset
Resolution: wontfix
Status: closednew
Triage Stage: Design decision neededAccepted

I'm reopening this ticket in the light of the introduction of explicit streaming responses, which removes the only objection to fixing this ticket.

I believe Content-Length should be added for non-streaming responses. Their content is consumed upon assignment. The overhead of computing response length will be negligible.

comment:11 by David Black, 9 years ago

Because some load balancers, such as Amazon "Elastic Load Balancing", require a content-length header django should at least document where the ConditionalGetMiddleware middleware may be required.

Last edited 9 years ago by David Black (previous) (diff)

comment:12 by Claude Paroz, 9 years ago

Has patch: set

comment:13 by Tim Graham, 9 years ago

Triage Stage: AcceptedReady for checkin

comment:14 by Claude Paroz <claude@…>, 8 years ago

Resolution: fixed
Status: newclosed

In 9588718:

Fixed #5897 -- Added the Content-Length response header in CommonMiddleware

Thanks Tim Graham for the review.

comment:15 by Patrick Cloke, 8 years ago

Cc: clokep@… added
Note: See TracTickets for help on using tickets.
Back to Top