Change LineDecoder to match stdlib splitlines, resulting in significant speed up

[giannitedesco]

Closes #2422

Leading to enormous speedups when doing things such as Response(...).iter_lines()

[tomchristie]

A solution using the built-in splitlines would probably be preferable to our own regex.
It might be worth taking a look at what requests does here?

[giannitedesco]

A solution using the built-in splitlines would probably be preferable to our own regex. It might be worth taking a look at what requests does here?

Actually requests is using splitlines, too. So should I just switch to that and update the failing tests?

[tomchristie]

So should I just switch to that and update the failing tests?

Switching to a splitlines based approach, using the requests implementation as a reference point would be a good starting point...

[giannitedesco]

Haha, yes, the first implementation I did with splitlines happens to be identical to theirs when refactored (ie. theirs is a generator, not based an object 😂 but, yeah, that broke the tests which is when I went off on a tangent to fix that.

[giannitedesco]

@florimondmanca @tomchristie is this acceptable? I suppose the benefit of this API is that if you do something like ''.join(resp.iter_lines()) you recover the original text and that means you could, for example, hash, encrypt, or compress the response body incrementally by lines without altering its contents? So, while incompatible, maybe it could be considered an improvement? But not sure the reasoning behind the previous normalization of line endings so who can say?

[tomchristie]

is this acceptable?

No. I don't believe resolving this issue should require the tests to change.

[tomchristie]

Hrm. I'd not appreciated that we have different behaviour from requests here. That's interesting.

(Our output includes trailing \n delimiters. Theirs does not.)

[giannitedesco]

Right, I feel like there are 3 options to what the behaviour could be:

1. no newlines (ala requests)
2. all newline types replaced with "\n" (ala httpx now)
3. all newlines preserved (result of this patch)

1 and 3 can be done efficiently and simply with "".splitlines(), and only 3 allows for reconstructing the exact original document after the fact (and therefore supports, eg. incrementally hashing a document line by line). (edit: i feel option 3 is the superior option since it supports the most use-cases)

Preservation of the existing semantics could be done with a regex perhaps, but it's tricky to get right. Or maybe a straight python implementation that just doesn't start from the beginning every time, it wouldn't be as fast as the ones which call to C code, but it could at least be linear-time?

Lemme know which direction you want to take it and I can give it a go..

[tomchristie]

Right, I feel like there are 3 options to what the behaviour could be.

I'd suggest that we've got the current behaviour wrong, and that we need a breaking API change here.
The sensible behaviour for iter_lines would be to match the same style as the sodlib's splitlines().
So, yeah, I think we want option (1) here. (Right?)

[ofek]

I think we want option (1) here. (Right?)

Yes imo

[florimondmanca]

After reading my comment pointing at a behavior change, it’s true that I would rather be expecting iter_lines() to return lines without newline separators at the end. This is what splitlines() does, but also other languages like Rust’s str.lines() (AdventOfCode season…), or JS when doing .split(/\r?\n/), etc.

What are the ways this change could impact existing code? I don’t see many. If code is currently using iter_lines() and calling trim() on the result, they shouldn’t be impacted, only those trim calls would become unnecessary. People can’t be relying on the current behavior to reconstruct the original content either, so there shouldn’t be breakage on this use case either.

[tomchristie]

Noticed that this implementation wouldn't return until flush for input that was just a sequence of \r characters, so I've adapted it slightly.

Implementation update in d3c6a8e.

Test change in 46cad89...

    decoder = LineDecoder()
    assert decoder.decode("") == []
    # This will now return `["a", "", "b"]` and *only* buffer the last portion.
    assert decoder.decode("a\r\rb\rc\r") == ["a", "", "b"]  
    assert decoder.flush() == ["c"]

[cdeler]

LGTM (with a small style suggestion which can be easily ignored)

[zanieb]

Can we also update this pull request title to note the change in behavior since it's no longer just a change in algo complexity?

[tomchristie]

Okay, I think we can merge this in now.
The line ending behaviour change means that we should bump the version on this one.

[ofek]

This is very exciting, thanks!