Repository navigation
Enhance HTTPResponse.readline() performance #63209
Description
| BPO | 19009 |
|---|---|
| Nosy | @orsenthil, @pitrou, @kristjanvalur, @bitdancer, @serhiy-storchaka |
| Files |
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:
assignee = None
closed_at = <Date 2014-03-19.16:07:11.511>
created_at = <Date 2013-09-13.13:39:30.424>
labels = ['library', 'performance']
title = 'Enhance HTTPResponse.readline() performance'
updated_at = <Date 2014-03-19.16:07:11.510>
user = 'https://github.com/kristjanvalur'bugs.python.org fields:
activity = <Date 2014-03-19.16:07:11.510>
actor = 'kristjan.jonsson'
assignee = 'none'
closed = True
closed_date = <Date 2014-03-19.16:07:11.511>
closer = 'kristjan.jonsson'
components = ['Library (Lib)']
creation = <Date 2013-09-13.13:39:30.424>
creator = 'kristjan.jonsson'
dependencies = []
files = ['31741', '31804', '31826', '31836', '31884']
hgrepos = []
issue_num = 19009
keywords = ['patch']
message_count = 24.0
messages = ['197574', '197587', '197595', '197598', '197601', '197603', '197604', '197950', '198142', '198148', '198157', '198163', '198166', '198171', '198172', '198173', '198212', '198446', '198447', '198469', '213963', '214071', '214107', '214108']
nosy_count = 6.0
nosy_names = ['orsenthil', 'pitrou', 'kristjan.jonsson', 'r.david.murray', 'python-dev', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'commit review'
status = 'closed'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue19009'
versions = ['Python 3.5']
Activity
kristjanvalur commentedon Sep 13, 2013
Some applications require reading http response data in "long" polls as it becomes available. This is used, e.g. to receive "notifications" over a HTTP stream.
Using response.read(large_buffer) is not possible because this will attempt to fullfill the entire request (using multiple underlying recv() calls), negating the attempt to get data as it becomes available.
Using "readline", and using \n boundaries in the data, this problem can be overcome.
Currently, readline is slow because it will attempt to read one byte at a time. Even if it is doing so from a buffered stream, it is still doing it one character at a time.
This patch adds a "peek" method to HttpResponse, which works both for chunked and non-chunked transfer encoding, thus improving the performance of readline. IOBase.readline will use peek() to determine the readahead it can use, instead of one byte which it must use by default.
pitrou commentedon Sep 13, 2013
This sounds ok on the principle. I suppose one can't simply wrap the "fp" inside a BufferedReader?
I think it would be good to add tests for the peek() implementation, though.
kristjanvalur commentedon Sep 13, 2013
The problem is that self.fp is already a Buffered stream, and such streams are documented to have their read() and readinto() calls make multiple system calls to fullfill the request.
My original goal was actually to make response.read(amt) not try to make multiple read() calls, so that one could have other delimiters than newline. It is simple for the chunked case, but I don't know how to bypass it for response.fp, since it is already a buffered file that has no guaranteed such behaviour.... wait, one can simply call peek() on it and know how much one can get :)
But anyway, the use case I have actually uses newlines as record separators in the http stream, so this enhancement seems less intrusive.
I'll add unit tests. There ought to be ready-made HTTPResponse tests already that I can use as templates.
pitrou commentedon Sep 13, 2013
Or you can use read1()?
bitdancer commentedon Sep 13, 2013
See also bpo-13541?
kristjanvalur commentedon Sep 13, 2013
Intersting. I didn't know about that. My excuse is that I never use 3.x except when I'm porting some CCP enhancements for cpython.
Here's a thought: HTTPResponse inherits from RawIOBase. Only the BufferedIO classes have read1() and are documented to have more than one system calls on their read() methods. Yet, HTTPResponse will happily behave in that way.
So, either HTTPResponse, being a non-buffered IO object, should not work in that way, or we could add a read1() method to it.
kristjanvalur commentedon Sep 13, 2013
I should add, I fully support the use case that response.read(amt=None) needs to read to the end of the response. It is only the read(amt=bufsize) use case I'm thinking of, and that could be handled with a read1() method.
kristjanvalur commentedon Sep 16, 2013
Ok, I'm adding a more comprehensive patch. It adds support for
peek, readline, and read1 for both regular and chunked responses.
readline falls back to IOBase.readline for chunked, which again makes use of peek() for performance.
read1() is available for other kinds of client buffering, e.g. when reading the stream incrementally using other delimiters than newline.
The real work is done by the _read1_or_peek_chunked() method. the code for both cases is almost identical so a dual purpose method is warranted.
Added test cases to test this. The test cases don't attempt to test that we don't block on partial resopnses, though. I'm not sure how I would write such a test. I'd probably have to extend FakeSocket to work on some stream that can be appended to...
serhiy-storchaka commentedon Sep 20, 2013
After adding read1() and peek() what stop us from inheriting HTTPResponse from BufferedIOBase?
I suggest split _read1_or_peek_chunked() by two parts. First part calculates n bounded by chunk_left (it can read the next chunk size and close a connection if needed). Perhaps it can be reused in _readall_chunked() and _readinto_chunked(() too. Second part which is controlled by boolean parameter should be moved out in read1() and peek().
kristjanvalur commentedon Sep 20, 2013
Ok, I have refactored this a bit.
A separate new function now takes care of the reading of chunk-header and tail. This simplifies the other functions.
I'm not sure what you mean by inheriting from the buffered class. Do we gain anything by doing that, would it change the code? or would it merely be for the benefit of ABC?
Note that the chunk protocol was wrong and I fixed the unittests: The final chunk is a _valid_ zero length chunk, i.e. 0\r\n\r\n. It contains two eol tokens like other chunks, one at the end of the length, the other at the end of the(null) payload.
serhiy-storchaka commentedon Sep 20, 2013
Good. It is even better than I expected.
Yes, it for the benefit of ABC. Some code tests isinstance(file, io.BufferedIOBase) and wraps stream in BufferedReader if it is false.
How new code processes invalid final chunk 0\r\n? We need test to check that there is no degradation.
Perhaps it relates to behavior change which I mention in the comment on Rietveld.
kristjanvalur commentedon Sep 20, 2013
Ok, I can change the base class inheritance and see if it changes things.
Note for the different interpretation of the final chunk:
Chunked encoding allows for adding headers after the final chunk. This is what _read_and_discard_trailer() does, but discarding the trailing headers. So, if support is ever added for reading those trailing headers, then we need make sure that we consume the last chunk correctly.
It is, of course, a matter of resilience, v.s. correctness, but the lesson of Internet Explorer should tell us to prefer correctness over flexibility, I think.
We can, of course, change this back. if you think. It is ieasier now, that the chunk consumer is in a single method.
pitrou commentedon Sep 20, 2013
I think what Serhiy is proposing is that we also succeed when the
server sends a truncated last chunk (and I agree with him).
kristjanvalur commentedon Sep 20, 2013
Ok, I can make it resilient. I was just pointing out that resilience in the face of RFC violations can be a bad thing. E.g. Internet explorer and how they allowed internet servers of the world to be lax in how they served their data.
No matter, I can allow truncated tails when reading chunks. But I don't think we ought to make that a requirement or make special unit tests for that. Either you read chunked encoding correctly or you don't. There are so many different ways you can break protocol, that this particular one is really only a special case.
pitrou commentedon Sep 20, 2013
I'm afraid the ship has sailed a long time ago on that. Poor HTTP
implementations are everywhere.
Well, the unit tests already exist, which is why I think we shouldn't
stop supporting that.
kristjanvalur commentedon Sep 20, 2013
Very well, let's support both then :)
kristjanvalur commentedon Sep 21, 2013
Ok, Here is a new patch.
We now inherit from BufferedIOBase. We must implement read(amt) ourselves, since the base class does not do it.
I was wrong about the final chunk, it is in fact 0\r\n. A final \r\n is then added to signal the end of the optional trailers.
Added a bunch of tests for chunked encoding, plus tests to verify http synchronization for keepalive connections.
serhiy-storchaka commentedon Sep 26, 2013
Kristján, did you notice my comments on Rietveld?
kristjanvalur commentedon Sep 26, 2013
Ah no actually. Thanks . I'll respond soon.
kristjanvalur commentedon Sep 27, 2013
A new patch set, in response to Serhiy's comments on Rietveld.
serhiy-storchaka commentedon Mar 18, 2014
LGTM.
python-dev commentedon Mar 19, 2014
New changeset 49017c391564 by Kristján Valur Jónsson in branch 'default':
Issue bpo-19009
http://hg.python.org/cpython/rev/49017c391564
pitrou commentedon Mar 19, 2014
If this is committed, should the issue be closed?
kristjanvalur commentedon Mar 19, 2014
Sure. If there are issues we'll just reopen. Closing.