Opened 3 years ago

Closed 2 years ago

#18972 closed Bug (fixed)

Refactor bundled WSGI server's chuking logic

Reported by: amosonn@… Owned by: woodm
Component: HTTP handling Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

on core/servers/basehttp.py, lines 97-103:

if length > 33554432:

offset = 0
while offset < length:

chunk_size = min(33554432, length)
self._write(data[offset:offset+chunk_size])
self._flush()
offset += chunk_size

Since only offset is increased, chunksize will always be 33554432, and on the last write might not be the actual amount written. Probably
chunk_size = min(33554432, length-offset)
is better.

Attachments (1)

0001-Fixed-18972-Refactored-bundled-wsgi-server-s-chunkin.patch (6.0 KB) - added by charettes 2 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 3 years ago by ptone

  • Component changed from Core (Other) to HTTP handling
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Cleanup/optimization to Bug
  • Version changed from 1.4 to master

Since this was originally written in 2007 - it would be worth looking any improved approaches to this chunking using more recently available Python features.

comment:2 Changed 3 years ago by dominik

  • Has patch set

https://github.com/django/django/pull/415

./runtests.py --settings=test_sqlite
returns OK

comment:3 Changed 3 years ago by datagrok

  • Keywords sprint2013 added
  • Owner changed from nobody to datagrok
  • Status changed from new to assigned

comment:4 Changed 3 years ago by datagrok

  • Keywords sprint2013 removed
  • Owner datagrok deleted
  • Status changed from assigned to new

Patch provided above did not include new unit tests.

Rather than send this back marked "needs tests" I created a unit test: https://github.com/django/django/pull/780

I also created an alternative implementation of a bug fix that I suspect may be more efficient: https://github.com/django/django/pull/782 .

Since I contributed code, I cannot bump this to "Ready for Check-In."

I suggest previous patch contributor should resubmit their patch with unit tests, or just pull in the one I made which should be sufficient.

This should still be easy pickings, next owner should be able to simply review and pick which implementation they prefer.

Version 0, edited 3 years ago by datagrok (next)

comment:5 Changed 2 years ago by woodm

  • Owner set to woodm
  • Status changed from new to assigned

comment:6 Changed 2 years ago by woodm

  • Triage Stage changed from Accepted to Ready for checkin

[REDACTED]

Wrong Ticket. :-(

Last edited 2 years ago by woodm (previous) (diff)

comment:7 Changed 2 years ago by woodm

  • Triage Stage changed from Ready for checkin to Accepted

comment:9 Changed 2 years ago by charettes

Reviewed the PR.

comment:10 Changed 2 years ago by woodm

Updated the PR to reflect charettes' suggestions.

comment:11 Changed 2 years ago by woodm

Heh, updated the PR to reflect Carl's suggestions as well.

comment:12 Changed 2 years ago by charettes

  • Summary changed from calculation blunder to Refactor bundled WSGI server's chuking logic

Added a rebased patch with work from woodm, datagrok and dominik that passes on python 2 and 3.

Note that I refactored the test to assert two chunks were written.

comment:13 Changed 2 years ago by charettes

  • Triage Stage changed from Accepted to Ready for checkin

@carljm reviewed the ticket. I will commit the patch soon.

comment:14 Changed 2 years ago by Simon Charette <charette.s@…>

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

In a7960bcb3575fd9fcd5180986ddcdba1caa46dd5:

Fixed #18972 -- Refactored bundled wsgi server's chunking algorithm.

Thanks to amosonn at yahoo.com for the report, @doda for the initial patch and
@datagrok for the revamped logic and test case.

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