bugfix2/ESP8266HTTPClient

[Jeroen88]

Because of git problems, start from a new fork and create a new PR. This was PR #6457

[Jeroen88]

See PR #6176 and PR #6457, this PR was merged while there was still an issue to solve.

With this PR:

::getString() no more returns an error but returns an empty String() if the server sends a '204 no content'
if a connection is being reused and this connection gets lost (this can be detected because e.g. ::GET() returns a value less than zero), the connection can be restored by calling ::end() followed by ::begin(). An example of this usage is added as ReuseConnectionV2.ino

This behavior of this example is tested with both a WiFiClient and a (BearSSL) WiFiClientSecure. The lost connection was simulated with switching off the router and switching it on again, and with stopping my HTTP(S) server and restarting it. In the first case a connection refused was reported, in the second case a connection lost. Switching the router back on or restarting the server caused the program to resume showing the ::GET() payload as expected.

[Jeroen88]

@d-a-v and @earlephilhower

Could you take a look at ::disconnect() here and here?
I think the two reset()'s should only be done if !preserveClient and should be added to the second block of code as well. What do you think?

[d-a-v]

@Jeroen88
Can you replace the former example by the new one ?

[Jeroen88]

@d-a-v I think I do not understand your request? Which example do you want to be replaced by what other example?

[d-a-v]

reuseConnectionV2 should replace reuseConnection.ino no ?

[Jeroen88]

@d-a-v I just checked the differences (because I forgot :)). They are minimal: in v2 ::begin() is called in setup() in stead of in the loop(), which is logical, because the connection is reused. Also, ::end(); ::begin() is called in the loop if an error occurred. Assumed is that the connection is lost and should be reinitialized.

So in short: the examples are different but v2 could replace the original example. However: I lost this branch and I am no git hero. Could you do it?

[earlephilhower]

See above comment about the reuse flag.

[earlephilhower]

Ping @Jeroen88. Would you be able to see about the merge conflict and the review?

[d-a-v]

@Jeroen88 can you give a look to your updated PR ?

edit: if you've lost your branch, here are the steps to recover it:

assuming your remotes names are origin for your fork, and upstream for this repository

$ git checkout master
$ git checkout -b bugfix2/ESP8266HTTPClient
$ git pull origin bugfix2/ESP8266HTTPClient

[devyte]

The actual core code change seems ok at a glance, but needs testing.
The example needs to be fixed.
BTW, the thinking that a failed connection should be handled by ::end() followed by ::begin() is correct in my view, the example is pretty good showcasing that.

[earlephilhower]

I used a simple Python3 script to try out the changes (enabling and disabling the protocol_version line:

#!/usr/bin/env python3
from http.server import HTTPServer, BaseHTTPRequestHandler

class SimpleHTTPRequestHandler(BaseHTTPRequestHandler):
    protocol_version = 'HTTP/1.1'
    def do_GET(self):
        resp = b'You asked: "' + bytes(self.requestline, 'utf-8') + b'"';
        self.send_response(200)
        self.send_header('Content-Length', str(len(resp)))
        self.end_headers()
        self.wfile.write(resp)


httpd = HTTPServer(('', 8000), SimpleHTTPRequestHandler)
httpd.serve_forever()

In master I do see connections reused even in HTTP/1.0 (a definite failure), but with this PR I confirm the connection is closed after each connection. So one bug is fixed.

I get, however, reuse in HTTP/1.0 when an error occurs, so this is still borked somewhere. The _reuse and _canReuse are funked up somewhere still.

I have made requests for /test return 500 and /taco return 200 codes. The 8266 requests /test and then /taco on the same HTTPClient object and the debug logs show that the 500 error code left the object thinking the TCP connection was still open(i.e. reuse), even though it should always be disconnected when a HTTP/1.0 server response happens.

[HTTP] begin...
[HTTP-Client][begin] url: http://192.168.1.6:8000/test
[HTTP-Client][begin] host: 192.168.1.6 port: 8000 url: /test
[HTTP] GET...
[HTTP-Client][sendRequest] type: 'GET' redirCount: 0
[HTTP-Client] connected to 192.168.1.6:8000
[HTTP-Client] sending request header
-----
GET /test HTTP/1.1
Host: 192.168.1.6:8000
User-Agent: ESP8266HTTPClient
Accept-Encoding: identity;q=1,chunked;q=0.1,*;q=0
Connection: keep-alive
Content-Length: 0

-----
[HTTP-Client][handleHeaderResponse] RX: 'HTTP/1.0 500 Internal Server Error
'
[HTTP-Client][handleHeaderResponse] RX: 'Server: BaseHTTP/0.6 Python/3.8.2
'
[HTTP-Client][handleHeaderResponse] RX: 'Date: Fri, 15 May 2020 05:01:05 GMT
'
[HTTP-Client][handleHeaderResponse] RX: '
'
[HTTP-Client][handleHeaderResponse] code: 500
[HTTP] GET... code: 500
[HTTP-Client][begin] url: http://192.168.1.6:8000/taco
[HTTP-Client][begin] host: 192.168.1.6 port: 8000 url: /taco
[HTTP-Client][sendRequest] type: 'GET' redirCount: 0
[HTTP-Client] connect: already connected, reusing connection
[HTTP-Client][returnError] error(-2): send header failed
[HTTP-Client][end] tcp is closed

[earlephilhower]

I think the HTTP/1.0 stuck open errors after a 500 (and probably in other cases) are basic issues with the way HTTPClient handles the TCP connection.

HTTPClient never actually closes the TCP connection on its own. It will leave the TCP connection open unless you explicitly do a getString which makes a StreamString and stuffs it with the HTTP server response, at which point the HTTP server itself will close the connection.

If you check the HTTP error code and find failure, unless you do a getString and throw it away, it won't disconnect.

-edit-
Tested the following change and it fixes the problem noted above by ensuring we always reconnect unless both _reuse and canReuse are true:

Arduino/libraries/ESP8266HTTPClient/src/ESP8266HTTPClient.cpp

Line 1209 in c4e0cbe

if(connected()) {

to

    if(_reuse && _canReuse && connected()) {

[earlephilhower]

Tested my custom HTTP/1.0 and /1.1 w/error 500 and w/o and it looks clean now.

Built and ran the example, works, too.