classification
Title: BaseHTTPRequestHandler innefficient when sending HTTP header
Type: performance Stage: resolved
Components: Library (Lib) Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: orsenthil Nosy List: brunogola, endian, gregory.p.smith, orsenthil, python-dev, r.david.murray, terry.reedy, yangman
Priority: normal Keywords: patch

Created on 2008-08-27 23:43 by yangman, last changed 2011-05-09 15:37 by Arfrever. This issue is now closed.

Files
File name Uploaded Description Edit
issue3709.diff endian, 2010-11-20 20:09 review
issue3709-from86665.diff endian, 2010-11-22 02:14 review
Messages (16)
msg72051 - (view) Author: Yang Zhao (yangman) Date: 2008-08-27 23:43
send_header() in BaseHTTPRequestHandler currently does a write to socket
every time send_header() is called. This results in excessive number of
TCP packets being regenerated. Ideally, as much of the HTTP packet is
buffered as possible, but, at minimum, the header should be sent with a
single write as there is a convenient end_header() functional available.

Behaviour is observed under python 2.5, but the related code looks
identical in SVN trunk.

Will contribute patch if request is deemed reasonable but no one is
available to work on it; I just need a few days.
msg73551 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2008-09-22 01:20
Buffering up header IO and sending it all at once is always a good thing
to do.  A patch and unit test would be greatly appreciated.
msg107973 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2010-06-17 00:58
Does this issue apply to 3.1/2?
msg107989 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2010-06-17 02:42
Yes, it is applicable to 3.1 and 3.2 as well. This is definitely a good to have performance improvement.
msg121717 - (view) Author: Andrew Schaaf (endian) Date: 2010-11-20 18:52
How about this patch?
msg121719 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-11-20 19:04
It looks good, but as mentioned on IRC it would be nice to have a unit test that confirmed that the headers were being correctly buffered.
msg121721 - (view) Author: Andrew Schaaf (endian) Date: 2010-11-20 19:05
Working on it...
msg121750 - (view) Author: Andrew Schaaf (endian) Date: 2010-11-20 20:09
Ready for review. Added test_header_buffering which fails without the BaseHTTPRequestHandler patch and passes with it.
msg121759 - (view) Author: Bruno Gola (brunogola) Date: 2010-11-20 20:29
applied the patch on py3k on trunk and everything seems fine (works here)
msg121815 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-11-20 23:48
Thanks for the patch and test.  They look good.

A doc update is also needed, since the docs are currently written in such a way that buffering the header lines makes the documentation no longer true.

Also, send_response_only writes directly to the output stream, it doesn't call send_header.  This breaks the buffering.  It also means that more test cases are needed, since the added test didn't catch this case.
msg121933 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2010-11-21 14:39
Fixed this revision 86640.

- Even though this is an internal optimization,I don't think back-porting is a good idea, because it changes the behavior of certain methods like send_header and end_headers

- Added the Documentation and other test scenario which was suggested by RDM.

'endian': In the NEWS entry, I have added your handle. If you want me to mention your Real Name, let me know.
msg121937 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-11-21 15:15
Senthil, I didn't clearly express my concern about send_response_only.  It doesn't look to me like, with buffering in place, that it *should* write directly, it looks to me like it should write to the buffer.  Consider specifically the fact that send_response_only is called from send_response_only, which is in turn called from send_error.

So IMO the unit test should at a minimum test that send_response_only buffers, and ideally it should test that send_error buffers (but that would require a somewhat different test harness since it would need to report that its write method was called only once).

I think the code you committed is still valid, it just doesn't quite do full header buffering.

And I agree that this should not be backported.
msg121943 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2010-11-21 15:50
On Sun, Nov 21, 2010 at 03:15:06PM +0000, R. David Murray wrote:
> Senthil, I didn't clearly express my concern about
> send_response_only.  It doesn't look to me like, with buffering in
> place, that it *should* write directly, it looks to me like it
> should write to the buffer.

Correct. Now that I re-looked at the code, keeping your above
statement in mind", I tend to agree with you and notice the problem. I
think, the headers_buffer should be moved higher up in the class so
that it buffer the headers from all these methods which atm write
directly to the output stream.
msg122066 - (view) Author: Andrew Schaaf (endian) Date: 2010-11-22 02:14
This patch (to 86665)...
* adds buffering to send_response_only
* adds flush_headers and uses it in end_headers and CGIHTTPRequestHandler.run_cgi
* tests {send_error,send_response_only,send_header} using a write-counting wfile
* improves the docs

Also, I've added my name to this account.
msg135598 - (view) Author: Roundup Robot (python-dev) Date: 2011-05-09 15:25
New changeset 25298224cb25 by Senthil Kumaran in branch 'default':
Issue #3709: a flush_headers method to BaseHTTPRequestHandler which manages the
http://hg.python.org/cpython/rev/25298224cb25
msg135599 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2011-05-09 15:26
Added the flush_headers method and the test function. this issue can be closed now. Thanks, Andrew Schaaf.
History
Date User Action Args
2011-05-09 15:37:56Arfreversetversions: + Python 3.3, - Python 3.2
2011-05-09 15:26:38orsenthilsetstatus: open -> closed

messages: + msg135599
2011-05-09 15:25:16python-devsetnosy: + python-dev
messages: + msg135598
2010-11-22 02:14:17endiansetfiles: + issue3709-from86665.diff

messages: + msg122066
2010-11-21 15:50:45orsenthilsetmessages: + msg121943
2010-11-21 15:15:04r.david.murraysetstatus: closed -> open

messages: + msg121937
2010-11-21 14:39:12orsenthilsetstatus: open -> closed
versions: - Python 3.1, Python 2.7
messages: + msg121933

assignee: gregory.p.smith -> orsenthil
resolution: fixed
stage: patch review -> resolved
2010-11-20 23:48:27r.david.murraysetstage: patch review
2010-11-20 23:48:13r.david.murraysetmessages: + msg121815
2010-11-20 20:29:15brunogolasetnosy: + brunogola
messages: + msg121759
2010-11-20 20:11:23endiansetfiles: - issue3709.diff
2010-11-20 20:11:19endiansetfiles: - issue3709.diff
2010-11-20 20:09:45endiansetfiles: + issue3709.diff

messages: + msg121750
2010-11-20 19:05:44endiansetfiles: + issue3709.diff
2010-11-20 19:05:01endiansetmessages: + msg121721
2010-11-20 19:04:15r.david.murraysetnosy: + r.david.murray
messages: + msg121719
2010-11-20 18:55:25eric.araujosetversions: - Python 2.6
2010-11-20 18:52:12endiansetfiles: + issue3709.diff

nosy: + endian
messages: + msg121717

keywords: + patch
2010-06-17 02:42:32orsenthilsetnosy: + orsenthil

messages: + msg107989
versions: + Python 3.1, Python 3.2
2010-06-17 00:58:09terry.reedysetnosy: + terry.reedy

messages: + msg107973
versions: + Python 2.7, - Python 2.5
2008-09-22 01:20:48gregory.p.smithsetpriority: normal
assignee: gregory.p.smith
messages: + msg73551
nosy: + gregory.p.smith
versions: + Python 2.6
2008-08-27 23:43:46yangmancreate