umbrella ticket to resolve iteration / read size / chunked encoding questions

[slingamn]

This ticket is intended to aggregate previous discussion from #539, #589, and #597 about the default value of chunk_size used by iter_content and iter_lines.

cc @mponton @gwrtheyrn @shazow

Issues:

1. The default read size of iter_content is 1 byte; this is probably inefficient
2. Requests does not expose the ability to read chunked encoding streams in the "correct" way, i.e., using the provided octet counts to tell how much to read.
3. However, this would not be suitable as the default implementation of iter_content anyway; not all websites are standards-compliant and when this was tried it caused more problems than it solved.
4. The current default read size for iter_lines is 10kB. This is high enough that iteration over lines can be perceived as unresponsive --- no lines are returned until all 10kB have been read.
5. There is no "correct" way to implement iter_lines using blocking I/O, we just have to bite the bullet and take a guess as to how much data we should read.
6. There's apparently some nondeterminism in iter_lines, I think because of the edge case where a read ends between a \r and a \n.
7. iter_lines is backed by iter_content, which operates on raw byte strings and splits at byte boundaries. I think there may be edge cases where we could split the body in the middle of a multi-byte encoding of a Unicode character.

My guess at a solution:

1. Set the default chunk_size to 1024 bytes, for both iter_content and iter_lines.
2. Provide a separate interface (possibly iter_chunks) for iterating over chunks of pages that are known to correctly implement chunked encoding, e.g., Twitter's firehose APIs
3. We may need our own implementation of splitlines that is deterministic with respect to our chunking boundaries, i.e., remembers if the last-read character was \r and suppresses a subsequent \n. We may also need to build in Unicode awareness at this level, i.e., decode as much of the body as is valid, then save any leftover invalid bytes to be prepended to the next chunk.

Comments and thoughts are much appreciated. Thanks for your time!

[kennethreitz]

👍 for iter_chunks

[kennethreitz]

@slingamn thanks so much for stepping up with this ;)

[mponton]

I'm not sure using 1024 as the default size for iter_lines is appropriate, I think it should be 1 byte. Don't get me wrong, I KNOW how bad this is for performance. But I stick with my original reasoning: If someone iterates on iter_lines, lines should be processed as soon as they are available and there should be no chance of "hanging" with complete lines in the buffer. The interface is called iter_lines so it should do just that by default.

A knowledgeable developer that wants better performance could still decide to use a larger read size knowing that it could mean that the read process will hang until the specified amount is read or the stream closed and that some lines could come in late.

I love requests because it is fun and easy to use first, yet very powerful. This is why I think iter_lines should behave as expected even if it means poor performance. This decision should simply be documented. Newcomers will appreciate this (i.e. works as expected out of the box) while more experienced devs can still fiddle with the buffer size.

That said, iter_content should definitely have a larger default read size. The previous one was 10K if I remember correctly. The newly proposed one is 1K. I'm not sure which one make a better default.

+1 for iter_chunks.

[shazow]

I think I'm missing something but why must iter_lines hang when it has complete lines in the buffer? Why can't we scan for completed lines (\n or such) and yield it just the same even if we've read less than the default size?

[mponton]

@shazow Because the synchronous network read call will not return until the specified amount of bytes has been read or the stream is closed. Thus, is you use a 1K read size and the server returns lines averaging 100 bytes, you will have to wait for the server to return at least 11 lines before iter_lines return any lines and your Python code is executed. If one line is sent every minute, it will take 11 minutes before your code is even aware of the first line sent by the server.

[shazow]

Ah I see what you mean. For some reason I was thinking in local IO terms. Thanks for the clarification!

I wonder if there is any way to do it timeout-based, such that it will read as much as it can in N ms and process, loop.

[slingamn]

@mponton these are good points.

Does anyone know what browsers typically do when reading streaming text data without chunked encoding?

[mponton]

I did a quick test out of curiosity. It seems to depend on the Content-Type. For text/ascii, each line is printed as it comes in without delay (Safari 6). If content type is unspecified or HTML, the browser waits for the server to close the connection before rendering the content.

In any case, I'm not sure we should compare and try to emulate a browser... We're "stuck" in synchronous I/O land, browsers aren't. We should implement what makes the most sense for requests.

[slingamn]

I was poking around for something unrelated and there's quite an interesting implementation of a readline method inside the socket module. I haven't read it in detail yet, but it looks as though it reads byte-at-a-time in some cases and 8kB at a time in others.

It doesn't respect unicode or universal newlines, so we probably can't use it out of the box, but it looks like they put quite a bit of thought into it, so maybe some of the ideas are applicable.

[gazpachoking]

It seems most of the discussion is to do with how iter_lines should work. Is there any reason for iter_content not to have a larger default chunk size? The performance hit was really hard for the 1 byte size before I figured out what was going on.

[slingamn]

There's no reason (at least, for iter_content's own default, as opposed to the default that iter_lines uses it with). I've been sort of reluctant to just dive in and try to change it, lest we trigger latent bugs (which is what happened the last time I changed a default).

To be honest, I think the typical use case should just be to read the entire body with the .content property. iter_content strikes me as a difficult interface to use correctly.

[mponton]

I had changed back to 10K (original value) in #589 but the travisbot failed the build because one of the unit tests' expected result was not the same with a chunk size > 1 (see https://github.com/kennethreitz/requests/pull/589#issuecomment-5538019 for details).

Kenneth, proceeded to close the issue saying iter_content now had a proper size. At the time I thought he had made the changes on his side would merge them. Apparently this feel through a crack or something...

[piotr-dobrogost]

@slingamn Could you elaborate on point 3 from your initial list?