-
-
Notifications
You must be signed in to change notification settings - Fork 31.7k
BaseHTTPRequestHandler innefficient when sending HTTP header #47959
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
send_header() in BaseHTTPRequestHandler currently does a write to socket Behaviour is observed under python 2.5, but the related code looks Will contribute patch if request is deemed reasonable but no one is |
Buffering up header IO and sending it all at once is always a good thing |
Does this issue apply to 3.1/2? |
Yes, it is applicable to 3.1 and 3.2 as well. This is definitely a good to have performance improvement. |
How about this patch? |
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. |
Working on it... |
Ready for review. Added test_header_buffering which fails without the BaseHTTPRequestHandler patch and passes with it. |
applied the patch on py3k on trunk and everything seems fine (works here) |
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. |
Fixed this revision 86640.
'endian': In the NEWS entry, I have added your handle. If you want me to mention your Real Name, let me know. |
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. |
On Sun, Nov 21, 2010 at 03:15:06PM +0000, R. David Murray wrote:
Correct. Now that I re-looked at the code, keeping your above |
This patch (to 86665)...
Also, I've added my name to this account. |
New changeset 25298224cb25 by Senthil Kumaran in branch 'default': |
Added the flush_headers method and the test function. this issue can be closed now. Thanks, Andrew Schaaf. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: