HTTPClient HTTP 1.1 persistence issue

[jpboucher]

Basic Infos

• This issue complies with the issue POLICY doc.
• I have read the documentation at readthedocs and the issue is not addressed there.
• I have tested that the issue is present in current master branch (aka latest git).
• I have searched the issue tracker for a similar issue.
• If there is a stack dump, I have decoded it.
• I have filled out all fields below.

Platform

• Hardware: [ESP-8266-wroom2]
• Core Version: [latest git hash]
• Development Env: [Arduino IDE]
• Operating System: [Ubuntu]

Settings in IDE

• Module: [Generic ESP8266 Module]
• Flash Size: [2MB (1MB SPIFFS)]
• lwip Variant: [v2 Lower Memory]
• Reset Method: [ck|nodemcu]
• Flash Frequency: [40Mhz]
• CPU Frequency: [80Mhz]
• Upload Using: [SERIAL]
• Upload Speed: [115200]

Problem Description

HTTPClient's Persistence support does not seem to follow the RFC 2068 standard for HTTP 1.1 but seems to follow HTTP 1.0 instead.

The condition to prevent disconnection is inside the HTTPClient::disconnect() method itself:

Arduino/libraries/ESP8266HTTPClient/src/ESP8266HTTPClient.cpp

Line 431 in a4ae856

if(_reuse && _canReuse) {

Because the disconnect method is called at the end of HTTPClient::writeToStream() and because writeToStream() is called by HTTPClient:getString(), you can only keep persistence if you set the _reuse and _canReuse flags.
The only way to set the _reuse flag is by calling HTTPClient::setReuse() and the only way to set the _canReuse flag is by receiving "Connection: Keep-Alive". (See HTTPClient::handleHeaderResponse()) Here lies the issue.

In HTTP 1.1, both the client and server are to assume that the connection will be persistent unless specified otherwise. The client needing the server to send "Connection: Keep-Alive", to keep the connection alive, is a behavior only found in HTTP 1.0 when persistence was not the default.

This means that a HTTP 1.1 server would not respond with a Keep-Alive if it was informed that the client is in HTTP 1.1.

With the current implementation I see no way of preserving persistence in the default HTTP 1.1 mode.

[earlephilhower]

@jpboucher, good writeup. Looks like you've gone over the code pretty well, and it seems like a small change.

Would you like to try writing a PR for this? You obviously have a failing case to test against, an understanding of the protocol, and have gone through the existing code.

[Jeroen88]

I have no time for a PR and testing, but I think the necessary changes are as simple as:

Insert after line 1250:
_canReuse = !_useHTTP10;

Insert after line 1268:
_canReuse = (_returnCode != '0');

[jpboucher]

@Jeroen88 I have checked and tried your proposition and it works.

I would make further improvements to be more in line with HTTP 1.1 though. At least for the keep-alive side of things.

I would also change the default value of the _reuse flag by adding the following in the constructor:

if(!_useHTTP10){
   _reuse = true;
}

[devyte]

@Jeroen88 @jpboucher If one of you makes a tested PR, and the other confirms it working, we can merge the proposed changes.

[Jeroen88]

@jpboucher nice that it works and that you have tested it! The initialisation in the constructor is not necessary, ::handleHeaderResponse() initialises it with the proposed line 1250.

What other improvements are you referring at?

@devyte I'll try to make some time to do a PR next sunday. Maybe one for the ESP32 too, it has the same library HTTPClient.

[jpboucher]

@Jeroen88 The line proposed for 1250 initializes the _canReuse variable and not the _reuse variable. The reason I am proposing that change is so that users don't have to call setReuse(true) when using HTTP 1.1

The other improvements I was refering to are rather nit-picky such as modifying a few lines to stop sending "Connection: Keep-Alive" in 1.1 mode when _reuse is true as it is useless information but I felt this was a bit outside of the scope of this issue and it does not affect proper operation.

[Jeroen88]

@jpboucher I initialized the _reuse variable in the header file.

[Jeroen88]

@jpboucher, @earlephilhower, @devyte, I just created PR #6176 for this.

[d-a-v]

fixed by #6176, thanks @Jeroen88

[ThomasWaldmann]

espressif/arduino-esp32@7d78247#r39308827 can somebody please comment?

update: that was changed afterwards: espressif/arduino-esp32@f4acac4#diff-39b6d5e36cff20acc96b42ef19ad4deb

[devyte]

@ThomasWaldmann this repo has nothing to do with the esp32.

[ThomasWaldmann]

@devyte some of the library code comes from the same source and has the same issues and changes.

[earlephilhower]

@devyte, this is the same patch @Jeroen88 put in. It needs to be updated on the ESP32 side. On their side, the PR wasn't fixed like over here.

[devyte]

Understood.