Fix logic for non-keepalive connections

[ptrunley]

This fixes a problem that occurs on non-keepalive HTTP connections under certain timing conditions. The logic around deciding if a connection is keepalive is reversed in the case that the fix deals with. Because of this, the code will sometimes call _http_read_body with a zero length body on non-keepalive connections. A zero length read within _http_read_body is treated as end of data and the connection gets closed.

Since it is timing dependent, I've only noticed this on certain podcasts and not every time. Sometimes retrying the podcast will make it work. I've also noticed that with logging enabled it sometimes does not repo. Here's some podcasts I've noticed the problem:

http://sleepwithmepodcast.libsyn.com/rss
http://feeds.feedburner.com/thornmorris

[michaelherger]

@philippe44 - what do you think about this patch? As you did the previous change in there, I was wondering whether this would conflict with your approach.

[philippe44]

Yep ... I don't know what I was thinking of at that time. What I really wanted was to avoid the problem where, in a persistent connection, the whole body is already buffered when handling the headers. In that case, there will be no socket event generated and the body will be missed.
So the right test is "when the full body has been received and there is no connection header or the connection header is keep-alive, then call _http_read_body".

_http_read_body( $self->socket, $self, $args ) if ( (!$self->response->headers->header('Connection') || $self->response->headers->header('Connection') =~ /keep-alive/i ) && $self->socket->_rbuf_length == ($headers->content_length || 0) );

I also don't know why I was using $headers and $self->reponse->header, as I think they are the same. Using $headers is shorter but less clean as the $headers variable should probably not be considered valid at that time as some later code modification might change it between the moment it is used to populate $self->reponse and this test

[ptrunley]

I'll update my change with that and give it a try. Thanks everyone for taking a look.

[ptrunley]

I tried the new test and it exhibited the same problem. After further though I think the right test is simply to check for "keep-alive" (see new commit). The reason is that HTTP/1.0 defaults to non-keep-alive, and only if the response contains a keep-alive "Connection" header is it actually a persistent connection. Checking for an empty/missing "Connection" header would only make sense for HTTP/1.1 since the default is keep-alive.

Here's the debug log for one of the podcasts that was giving trouble. Note our request is HTTP/1.0 and that the response has no "Connection" header or "Content-Length". In this case we should treat it as non-keep-alive, but the revised test would treat it as keep-alive and force a premature zero byte read.

[18-11-01 21:23:17.0523] Slim::Networking::SimpleAsyncHTTP::_createHTTPRequest (110) GETing http://feeds.feedburner.com/thornmorris
[18-11-01 21:23:21.5361] Slim::Networking::Async::connect (108) Connecting to feeds.feedburner.com:80
[18-11-01 21:23:21.5493] Slim::Networking::Async::_async_connect (181) Slim::Networking::Async::Socket::HTTP=GLOB(0x851f398) => 46 connected, ready to write request
[18-11-01 21:23:21.5496] Slim::Networking::Async::write_async (211) Sending: [GET /thornmorris HTTP/1.0
Connection: close
Cache-Control: no-cache
Accept: /
Accept-Encoding: deflate, gzip
Accept-Language: en
Host: feeds.feedburner.com
User-Agent: Mozilla/5.0 (Linux; N; Red Hat; x86_64-linux; EN; utf8) SqueezeCenter, Squeezebox Server, Logitech Media Server/7.9.2/git-892b66f
Icy-Metadata:

]
[18-11-01 21:23:21.6099] Slim::Networking::Async::HTTP::_http_read (357) Headers read. code: 200 status: OK
[18-11-01 21:23:21.6105] Slim::Networking::Async::HTTP::_http_read (358) bless({
"cache-control" => "private, max-age=0",
"content-encoding" => "gzip",
"content-type" => "text/xml; charset=UTF-8",
date => "Fri, 02 Nov 2018 04:23:21 GMT",
etag => "QZ56DhQ2kRrgeD3ZhhADFZ2Rzv0",
expires => "Fri, 02 Nov 2018 04:23:21 GMT",
"last-modified" => "Fri, 02 Nov 2018 03:12:33 GMT",
server => "GSE",
"x-content-type-options" => "nosniff",
"x-xss-protection" => "1; mode=block",
}, "HTTP::Headers")
[18-11-01 21:23:21.6109] Slim::Networking::Async::HTTP::_http_read_body (455) Read body: [5516] bytes
[18-11-01 21:23:21.6113] Slim::Networking::Async::HTTP::_http_read_body (455) Read body: [1418] byt

[philippe44]

I'm wondering then if we should not do a more complete test. An HTTP 1.1 connection w/o a Connection header should be considered persistent and in that case the current patch would break that, the same way my other proposal broke the HTTP 1.0
A keep-alive connection with a body but no content-length is an error, I think. So when there is a non-zero content-length equals to the received buffer and a connection header keep-alive or no connection header, we should call _http_read_body, no?

_http_read_body( $self->socket, $self, $args ) if ( (!$self->response->headers->header('Connection') || $self->response->headers->header('Connection') =~ /keep-alive/i ) && $headers->content_length && $self->socket->_rbuf_length == $headers->content_length );

[ptrunley]

Unless I'm misunderstanding the code the request will always be HTTP/1.0, there are several places in the module where the protocol is explicitly set to HTTP1.0. The comments state that HTTP/1.1 is not supported because there is no code to handle chunked encoding (which are always permitted in 1.1).

[philippe44]

Then you're right, I did not read enough the code. It took me a while to remember where I needed that. This is for ATV pairing, where the HTTP connection must be kept alive
On a side note, I wish the HTTP 1.1 and chunked-encoding was respected by all implementations. I've seen various cases, through my UPnPBridge of players that pretend to be 1.1 and do not handle chunked-encoding ...
Anyway, I'm digressing. Let's go ahead with your patch

[ptrunley]

Thanks @philippe44.

[philippe44]

Thanks to you - I feel bad I injected some crap in LMS' code :-(

[mherger]

Thanks for the constructive discussion. I learned a bit about our code 😀. We’re all good to merge this then?

[ptrunley]

Yup.

[mherger]

Thanks a lot, both of you!

[philippe44]

BTW, I found what was my original intention and why I thought it was working. It's for my AirPlay bridge: the pin-pairing negociation with ATV requires the session to be persistent, but when receiving an HTTP 1.0 with a "Connection:keep-alive", the ATV responds with no "Connection" header, so I needed that to be considered as a a valid keep-alive session.
I totally agree that the solution you've proposed is the right one and what ATV does is wrong, so I will continue to have my own HTTP.pm in the plugin to deal with that oddity
No need for any change, just wanted to explain

[ptrunley]

Makes sense. I've been wondering if the whole thing couldn't be simplified to simply call _http_read_body if _rbuf_length > 0. That should work for every case, without needing to determine if we are in a keep-alive situation or not. In fact I think this (or something like it) should be built-in to the Select logic automatically, rather than needing the caller to figure it out.

[philippe44]

I agree but these changes are scary as they can have lots of side effect and validation capabilities of LMS are thin (...)