Code

#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 13 months ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 19 months 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 19 months ago by dominik

  • Has patch set

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

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

comment:3 Changed 14 months ago by datagrok

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

comment:4 Changed 14 months ago by datagrok

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

Patch provided above did not include new unit tests.

Note that although there is a bug here (the chunking math is wrong) it does not actually break anything. (Since x[a:b] where b>len(x) simply truncates and does not error.)

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.

Last edited 14 months ago by datagrok (previous) (diff)

comment:5 Changed 13 months ago by woodm

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

comment:6 Changed 13 months ago by woodm

  • Triage Stage changed from Accepted to Ready for checkin

[REDACTED]

Wrong Ticket. :-(

Last edited 13 months ago by woodm (previous) (diff)

comment:7 Changed 13 months ago by woodm

  • Triage Stage changed from Ready for checkin to Accepted

comment:9 Changed 13 months ago by charettes

Reviewed the PR.

comment:10 Changed 13 months ago by woodm

Updated the PR to reflect charettes' suggestions.

comment:11 Changed 13 months ago by woodm

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

comment:12 Changed 13 months 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 13 months 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 13 months 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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.