Bugfix/esp8266 http client #6176

Jeroen88 · 2019-06-02T11:06:10Z

Fix HTTP1.1 persistence issue found by @jpboucher solving issue #6152. With HTTP1.1 default behavior now is to reuse the connection, which is compliant with specs.

libraries/ESP8266HTTPClient/src/ESP8266HTTPClient.cpp

earlephilhower

MklittleFS is still included in the PR. That's actually a .gitignore bug and I'll put a PR in to fix it.

Please double-check that header parsing, I think you're going to set canreuse to true even on HTTP 1.0 connections.

jpboucher · 2019-06-06T15:09:32Z

~~With this code _canReuse will be set to false in HTTP 1.0. Which is fine as it will only be set to true if keep-alive is received in the Connection token in HTTP 1.0.~~

I have more of a problem with the _reuse flag being set to true in the header file as it will be applied for both HTTP 1.1 and 1.0.

To fix that we would probably need to set _reuse to false when useHTTP10() is called and _useHTTP10 is set to true. And set it back to true if _useHTTP10 is set to false.

Jeroen88 · 2019-06-06T15:38:05Z

@earlephilhower you were right about the header parsing. This is corrected in this commit.

@jpboucher the connection is only kept alive if _reuse and _canReuse both are true, At the beginning of HTTPClient::handleHeaderResponse() _canReuse is initialized to !_useHTTP10, and later the response header is checked for 'Connection: Keep-Alive', so this is right

jpboucher · 2019-06-06T15:54:40Z

@Jeroen88 If you look at HTTPClient::sendHeader(const char * type) you will see that the _reuse flag is checked for sending keep-alive. If _reuse is true by default you would be sending Keep-alive in HTTP 1.0 even if you never called setReuse(). You are informing the server that you are reusing the socket even if you are not. This will cause issues.

Jeroen88 · 2019-06-06T16:07:35Z

@jpboucher I think you are right, I will check this later today and correct it

I will check for if(_reuse && !_useHTTP10), agree?

jpboucher · 2019-06-06T17:04:00Z

@Jeroen88 Which line? You could still use what I proposed: set _reuse to false inside useHTTP10() when _useHTTP10 is set to true. And set _reuse back to true if _useHTTP10 is set to false.

Jeroen88 · 2019-06-06T17:50:33Z

@jpboucher In ::sendHeader()

jpboucher · 2019-06-06T18:13:39Z

@Jeroen88 No, this would not work. You want to send Keep-alive in HTTP 1.0 but only if setReuse(true) was called. To keep it simple, the default value of _reuse should depend on the HTTP version being used. Which is why I think the best course of action is to change it's state inside useHTTP10(). It's alright that the default value in the header file is True since by default we are in HTTP1.1.

Jeroen88 · 2019-06-06T20:07:50Z

@jpboucher thnx for your insights, I followed your suggestion :)

Jeroen88 · 2019-06-06T22:30:16Z

MklittleFS is still included in the PR. That's actually a .gitignore bug and I'll put a PR in to fix it.

Please double-check that header parsing, I think you're going to set canreuse to true even on HTTP 1.0 connections.

@earlephilhower I do not know how to remove it from the PR, can you do that for me?

earlephilhower · 2019-06-06T22:46:01Z

@Jeroen88 Sorry for name typo before. I can't edit your repo where the PR actually lives, but it should not be hard for you. I think git rm on it will remove it, then you can repush. You might need to redo get.py if git wants to actually delete the dir.

Jeroen88 · 2019-06-07T06:03:17Z

@earlephilhower I can't get it done. I pull from the remote master to local master. Git says: everything is up-to-date. I push to my remote branch and then I checkout my bugfix/ESP8266HTTPClient branch. I merge local remote into the bugfix branch.
Even if I do not delete the extra file, when I push the branch to remote, I get:

 ! [rejected]          bugfix/ESP8266HTTPClient -> bugfix/ESP8266HTTPClient (fetch first)
error: failed to push some refs to 'https://github.com/Jeroen88/Arduino.git'
hint: Updates were rejected because the remote contains work that you do
hint: not have locally. This is usually caused by another repository pushing
hint: to the same ref. You may want to first integrate the remote changes
hint: (e.g., 'git pull ...') before pushing again.

git status says:

On branch bugfix/ESP8266HTTPClient
Your branch is ahead of 'origin/bugfix/ESP8266HTTPClient' by 4 commits.
  (use "git push" to publish your local commits)

nothing to commit, working tree clean

Any suggestions?

jpboucher · 2019-06-07T13:01:42Z

At first glance it seems that your remote bugfix branch evolved independently from your local branch somehow. You should try out git kraken (or any other git GUI) to seek differences between your remote and local branch. Have you tried pulling from you remote bugfix branch before pushing?

jpboucher · 2019-06-07T15:14:15Z

Another nitpick that would probably help the API is to add a lock on the _reuse variable when setReuse() is called. I don't think it's mandatory but I think it would be weird for users to call setReuse() only for the flag to be unexpectedly changed anyways when useHTTP10() is called.

…ugfix/ESP8266HTTPClient

Jeroen88 · 2019-06-07T15:29:54Z

@jpboucher thnx, with git kraken it was possible to solve the branch conflict :)

Jeroen88 · 2019-06-08T10:45:30Z

@jpboucher Could you suggest some lines of code?

d-a-v · 2019-08-05T16:03:44Z

in::end() there is a disconnect() too, this should be disconnect(false), right?

I think that too. end() semantic is when one want to call begin() again.

I am still not sure of your interpretation versus mine, especially because keep-alive is the default for HTTP/1.1. But I will follow your suggestion :)

You must not follow if you don't agree, because it's your PR.
Do you agree with:

        if(headerName.equalsIgnoreCase("Connection")) {
            if (headerValue.indexOf("close") >= 0 &&
                headerValue.indexOf("keep-alive") < 0) {
                _canReuse = false;
            }
        }

Jeroen88 · 2019-08-05T16:21:20Z

@d-a-v, @dmdt, I get a similar error like @dmdt after a while:

[HTTP-Client][handleHeaderResponse] RX: 'HTTP/1.1 200 OK'
[HTTP-Client][handleHeaderResponse] RX: 'X-Powered-By: Express'
[HTTP-Client][handleHeaderResponse] RX: 'Content-Type: application/json; charset=utf-8'
[HTTP-Client][handleHeaderResponse] RX: 'Content-Length: 574'
[HTTP-Client][handleHeaderResponse] RX: 'ETag: W/"23e-DB5ulCBY347Z4j19qxT6CsFyi84"'
[HTTP-Client][handleHeaderResponse] RX: 'Date: Mon, 05 Aug 2019 16:12:11 GMT'
[HTTP-Client][handleHeaderResponse] RX: 'Connection: close'
[HTTP-Client][handleHeaderResponse] RX: ''
[HTTP-Client][handleHeaderResponse] code: 200
[HTTP-Client][handleHeaderResponse] size: 574
Code: 200
[HTTP-Client][writeToStreamDataBlock] connection closed or file end (written: 574).
[HTTP-Client][end] tcp stop
Body: {"config_id":"1" ... etc}
[HTTP-Client][sendRequest] type: 'GET' redirCount: 0
[HTTP-Client] connect: HTTPClient::begin was not called or returned error
[HTTP-Client][returnError] error(-1): connection refused
Code: -1
Body: {"config_id":"1" ... etc}
[HTTP-Client][sendRequest] type: 'GET' redirCount: 0
[HTTP-Client] connect: HTTPClient::begin was not called or returned error
[HTTP-Client][returnError] error(-1): connection refused

Should this be handled by the client? Or should this be handled by the application? I assume that calling ::end() and then ::begin() will restore the connection, but I did not try it yet...

The logging comes from a HTTP/1.1 example with setReuse (false)

d-a-v · 2019-08-05T16:22:52Z

Isn't that fixed with disconnect(true) ?

Jeroen88 · 2019-08-05T16:24:11Z

@d-a-v Nope, this is with the code that I just committed. At first a lot of responses are correct, the logging I posted is when it goes wrong...

Jeroen88 · 2019-08-05T16:27:02Z

@d-a-v ::geString() still returns the previously fetched payload. ::clear() should be called earlier in the process?

d-a-v · 2019-08-05T17:58:16Z

There is a missing true L994. Without it I still have the error when forcing _canReuse to false (HTTP/1.0).

Jeroen88 · 2019-08-05T19:49:07Z

@d-a-v

Do you agree with:

    if(headerName.equalsIgnoreCase("Connection")) {
        if (headerValue.indexOf("close") >= 0 &&
            headerValue.indexOf("keep-alive") < 0) {
            _canReuse = false;
        }
    }

Yes, I do agree :)

Missing 'true' added.

I think I found another bug, in ::handleHeaderResponse() _canReuse should be initialized to _reuse and not to !_useHTTP10. In this way

    https.useHTTP10();
    https.setReuse(true);

works correctly.

I lost some work during pushing and pulling. I think I recreated everything, could you confirm? I did test HTTP/1.0 and HTTP/1.1, both with and without reusing the connection, but not the '_canReuse = false'-hack, and everything looks fine.

Jeroen88 · 2019-08-05T19:57:38Z

@d-a-v What about getting an http code < 0 indicating an error? I am running a test now, waiting for an error, and the call ::end() and ::begin() to restore the connection, to see if this works.

I think it should be handled in the .ino, not in the library, do you agree?

d-a-v · 2019-08-05T20:20:13Z

I get no more error with _canReuse=false faking http/1.0 👍

What about getting an http code < 0 indicating an error?

I think I don't understand the question:

all ::sendRequest return values are <0 on error, and
http code are normalized

I think it should be handled in the .ino, not in the library, do you agree?

Yes, error handling should be done in sketch, and demonstrated in examples.

Jeroen88 · 2019-08-05T20:30:52Z

@d-a-v That was my question, error handling should be done in the sketch :)

Thanks for the cooperation on this one!

Jeroen88 · 2019-08-06T07:33:14Z

@dmdt, @d-a-v, a sketch is running for more than 10 hours now, GETting data every 2 seconds without any error

Jeroen88 · 2019-08-06T07:37:30Z

@d-a-v Shouldn't we keep the default parameter for ::disconnect()? Every call from the library is with parameter now, and keeping the default parameter does not break any sketch.

d-a-v · 2019-08-06T13:17:44Z

Shouldn't we keep the default parameter for ::disconnect()?

I think it will give this PR a faster chance to get merged :)

Jeroen88 · 2019-08-06T13:52:57Z

@d-a-v Done!

Jeroen88 · 2019-08-13T16:26:19Z

@d-a-v I think I found another flaw in the ESP8266HTTPClient:

::getString() calls ::writeToStream() and at the end of this last function ::disconnect(true) is called. I think after every call to ::connect() a call to::disconnect() should be made. However, if ::sendRequest() returns an error, my sketch does not make a call to ::getString() (because it tests for a return code < 0) and then ::disconnect() is not called. Also, if the server returns 204, no content, getString() returns an error, because the client has already disconnected, and there is no data available.

Both errors can easily be corrected, but lead to the requirement to always call ::getString() (even if you know there is no content or if you e.g. read the result directly using ::getStreamPtr()) because otherwise the connection is not handled properly.

I do not like the above solution (although I am running tests on it now)

Another option would be to introduce a new function, e.g. ::stop() or any other name that should always be called after finishing reading the response, or require ::disconnect(true) to be called always. But the latter suggests that the client is disconnected while it might be reused...

I do not like this solution either.

Any ideas?

d-a-v · 2019-08-26T09:23:37Z

@Jeroen88
Can we simply add more ::disconnect(true) on 204 or failing ::sendRequest(), and/or internally read+cancel content (if needed, maybe with the help of a new bool set to true when user has read content, in ::sendRequest()) when user has not previously read content / called ::getString() ?

earlephilhower · 2019-08-26T13:56:12Z

libraries/ESP8266HTTPClient/src/ESP8266HTTPClient.cpp

@@ -563,6 +573,7 @@ void HTTPClient::setRedirectLimit(uint16_t limit)
 void HTTPClient::useHTTP10(bool useHTTP10)
 {
    _useHTTP10 = useHTTP10;
+    _reuse = !useHTTP10;


With this method and ::setReuse() it is possible to set useHTTP10 = true and _reuse=true. That is the user shooting themselves in the foot, which is not a high prio for me.

At some point might want to go back and see if useHTTP10 === !reuse (I think so after reading the code briefly) and then drop one or the other variable if that's true. But for this PR, I'm OK.

@earlephilhower for HTTP/1.0 the default is not reusing the connection, the default for HTTP/1.1 is reusing. However reusing is allowed also on HTTP/1.0. In this case the client must send a Connection: Keep-Alive request header. The server may respond with a Connection: Keep-Alive in which case the connection can be reused. In HTTP/1.1 the client may send a connection: Keep-Alive header, but this is not obligatory. The connection is supposed to be reused unless the server sends a connection: close response header.
This is exactly what ESP8266HTTPClient does

d-a-v · 2019-08-26T14:13:34Z

@Jeroen88 can you open a new issue with MCVE about your last message ?

Jeroen88 · 2019-08-26T19:03:57Z

@d-a-v I will later this week

Jeroen88 added 2 commits June 2, 2019 12:58

Got tools/mklittlefs/mklittlefs after submodule update --init

9600f3e

Solve HTTP1.1 persistance issue #6152

b42f5a4

Jeroen88 mentioned this pull request Jun 2, 2019

HTTPClient HTTP 1.1 persistence issue #6152

Closed

6 tasks

Merge branch 'master' into bugfix/ESP8266HTTPClient

2bc8695

earlephilhower reviewed Jun 6, 2019

View reviewed changes

libraries/ESP8266HTTPClient/src/ESP8266HTTPClient.cpp Outdated Show resolved Hide resolved

earlephilhower requested changes Jun 6, 2019

View reviewed changes

Jeroen88 added 3 commits June 6, 2019 17:31

fix checking the header

8fc039d

Merge branch 'master' of https://github.com/esp8266/Arduino

d532476

Merge branch 'master' into bugfix/ESP8266HTTPClient

6cc3c60

Jeroen88 added 2 commits June 6, 2019 22:01

Let reuse connection depend on protocol used: HTTP1.0 or HTTP1.1

f382aa6

Merge branch 'master' into bugfix/ESP8266HTTPClient

ed06407

Jeroen88 added 2 commits June 7, 2019 07:54

Merge branch 'master' of https://github.com/esp8266/Arduino

d45fe30

Merge branch 'master' into bugfix/ESP8266HTTPClient

cd7d7d5

Jeroen88 added 2 commits June 7, 2019 17:24

Merge remote-tracking branch 'origin/bugfix/ESP8266HTTPClient' into b…

9bd99ca

…ugfix/ESP8266HTTPClient

Removed tools/mklittlefs/mklittlefs

51dd084

Varios changes

0e3c92f

d-a-v approved these changes Aug 6, 2019

View reviewed changes

Restore ::disconnect() default parameter to false

293c68c

Jeroen88 added 3 commits August 7, 2019 18:03

Merge branch 'master' of https://github.com/esp8266/Arduino

96ecd06

Merge branch 'master' into bugfix/ESP8266HTTPClient

84ef3d3

Add return false in case of oom

82affca

Merge branch 'master' into bugfix/ESP8266HTTPClient

aeb8f3e

earlephilhower approved these changes Aug 26, 2019

View reviewed changes

d-a-v merged commit 60d519e into esp8266:master Aug 26, 2019

Jeroen88 deleted the bugfix/ESP8266HTTPClient branch August 27, 2019 16:01

This was referenced Aug 27, 2019

Bugfix/esp8266 http client #6457

Closed

bugfix2/ESP8266HTTPClient #6476

Merged

Bugfix/esp8266 http client #6176

Bugfix/esp8266 http client #6176

Conversation

Jeroen88 commented Jun 2, 2019

earlephilhower left a comment

Choose a reason for hiding this comment

jpboucher commented Jun 6, 2019 • edited Loading

Jeroen88 commented Jun 6, 2019

jpboucher commented Jun 6, 2019 • edited Loading

Jeroen88 commented Jun 6, 2019 • edited Loading

jpboucher commented Jun 6, 2019 • edited Loading

Jeroen88 commented Jun 6, 2019

jpboucher commented Jun 6, 2019

Jeroen88 commented Jun 6, 2019

Jeroen88 commented Jun 6, 2019

earlephilhower commented Jun 6, 2019

Jeroen88 commented Jun 7, 2019 • edited Loading

jpboucher commented Jun 7, 2019

jpboucher commented Jun 7, 2019

Jeroen88 commented Jun 7, 2019

Jeroen88 commented Jun 8, 2019

d-a-v commented Aug 5, 2019

Jeroen88 commented Aug 5, 2019

d-a-v commented Aug 5, 2019 • edited Loading

Jeroen88 commented Aug 5, 2019 • edited Loading

Jeroen88 commented Aug 5, 2019

d-a-v commented Aug 5, 2019 • edited Loading

Jeroen88 commented Aug 5, 2019

Jeroen88 commented Aug 5, 2019

d-a-v commented Aug 5, 2019

Jeroen88 commented Aug 5, 2019

Jeroen88 commented Aug 6, 2019

Jeroen88 commented Aug 6, 2019

d-a-v commented Aug 6, 2019

Jeroen88 commented Aug 6, 2019

Jeroen88 commented Aug 13, 2019

d-a-v commented Aug 26, 2019

earlephilhower Aug 26, 2019

Choose a reason for hiding this comment

Jeroen88 Aug 27, 2019

Choose a reason for hiding this comment

d-a-v commented Aug 26, 2019

Jeroen88 commented Aug 26, 2019

jpboucher commented Jun 6, 2019 •

edited

Loading

jpboucher commented Jun 6, 2019 •

edited

Loading

Jeroen88 commented Jun 6, 2019 •

edited

Loading

jpboucher commented Jun 6, 2019 •

edited

Loading

Jeroen88 commented Jun 7, 2019 •

edited

Loading

d-a-v commented Aug 5, 2019 •

edited

Loading

Jeroen88 commented Aug 5, 2019 •

edited

Loading

d-a-v commented Aug 5, 2019 •

edited

Loading