Skip to content

Undocumented negative return values from HTTPClient::get() #5137

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
teo1978 opened this issue Sep 15, 2018 · 34 comments
Closed

Undocumented negative return values from HTTPClient::get() #5137

teo1978 opened this issue Sep 15, 2018 · 34 comments

Comments

@teo1978
Copy link
Contributor

teo1978 commented Sep 15, 2018

All I can find in the documentation about HTTPClient::get() return value is that it returns "the http code".

Sometimes it returns -1. I guess that happens when something fails while sending the request, or parsing the response (if you even got one), so there's no http code to be returned.

However

  1. that should be documented
  2. There needs to be a way to get information about what the error actually was. Did the client fail to connect to the server? Did it fail to send the request? Did it get an invalid response? What else?
    If there already exists a way to get that kind of information, then that needs to be documented too.
@devyte
Copy link
Collaborator

devyte commented Sep 18, 2018

@teo1978 when you opened this issue, you were presented with an issue template that contains specific instructions and requested info. You have ignored this.
Closing due to lack of info.
If you think your issue is relevant, please open a new issue, read and follow the instructions, and fill out the requested info. In addition, make sure to include a MCVE sketch, and make sure it has a url that shows the issue, even if it happens only sometimes. If you used a private url, such as a webpage served from a private server that you set up yourself, then that needs to be explained in detail, including how to set up the server.

@devyte devyte closed this as completed Sep 18, 2018
@teo1978
Copy link
Contributor Author

teo1978 commented Sep 18, 2018

This is a documentation issue, you don't need a reproducible example.

What you need to do is look at the "return -1" statement in the function's code and document them.

Well actually there's point 2 which is not just documentation, that is more of a feature request. If you read it, you'll see you don't need (nor does it make sense to ask for) a reproducible case for that either.

@Pablo2048
Copy link

  1. Directly in ESP8266HTTPClient.h is
    /// HTTP client errors
    #define HTTPC_ERROR_CONNECTION_REFUSED (-1)
    #define HTTPC_ERROR_SEND_HEADER_FAILED (-2)
    #define HTTPC_ERROR_SEND_PAYLOAD_FAILED (-3)
    #define HTTPC_ERROR_NOT_CONNECTED (-4)
    #define HTTPC_ERROR_CONNECTION_LOST (-5)
    #define HTTPC_ERROR_NO_STREAM (-6)
    #define HTTPC_ERROR_NO_HTTP_SERVER (-7)
    #define HTTPC_ERROR_TOO_LESS_RAM (-8)
    #define HTTPC_ERROR_ENCODING (-9)
    #define HTTPC_ERROR_STREAM_WRITE (-10)
    #define HTTPC_ERROR_READ_TIMEOUT (-11)

  2. you can also call .errorToString() to get text representation of the error code given

@teo1978
Copy link
Contributor Author

teo1978 commented Sep 18, 2018

I see, thanks (I usually don't look at the source code for documentation).

So this is HTTPC_ERROR_CONNECTION_REFUSED.
Now, being a little skeptical about the connection being actually refused that often (given I've never observed a connection being refused by the same server from any other client - actually I've very rarely if ever observed any server refusing a connection for an http request and then accepting one for an identical http request, unless under veeeeery high load which is not the case here), I've had a look at the source code of GET() and in turn sendRequest, and I see:

int HTTPClient::sendRequest(const char * type, uint8_t * payload, size_t size)
{
    // connect to server
    if(!connect()) {
        return returnError(HTTPC_ERROR_CONNECTION_REFUSED);
    }

So I wonder: is it really safe to assume that, if HTTPClient::connect() return false, it's necessarily a "connection refused" error? I strongly doubt it.

@Pablo2048
Copy link

You can enable serial port debugging and see, where in ::connect() method is source of the error...

@teo1978
Copy link
Contributor Author

teo1978 commented Sep 22, 2018

You can enable serial port debugging and see, where in ::connect() method is source of the error...

I have tried that thanks to your suggestion, and it says more or less what we already knew: "connection refused":

[HTTP-Client][begin] url: http://******
[HTTP-Client][begin] host: ******.com port: 80 url: /******
[hostByName] request IP for: ******.com
[hostByName] Host: ******.com IP: ***.***.***.***
[HTTP-Client] failed connect to ******.com:80
[HTTP-Client][returnError] error(-1): connection refused
[HTTP-Client] getStreamPtr: not connected

I strongly suspect HttpClient is wrongly reporting as "connection refused" a broader class of connection failuers, but I'd like to verify that. Do you know if there are logs on the server (it's Debian 8) where I can look, where refused connections are logged?

I have also added WIFI to the debug level (besides HTTTP_CLIENT) but that doesn't add a bit of information when this error occurs.

@Pablo2048
Copy link

Pablo2048 commented Sep 23, 2018

Hmm, it seems like the problem is deeper than I thought (now it seems like problem in WiFiClient, or deeper). Can you please try to use direct ip address in your query so we can eliminate problem with DNS? It seems like DNS is not the source of the problem sooo - probably you have to add more debug info into int WiFiClient::connect(IPAddress ip, uint16_t port) method to see more...

@teo1978
Copy link
Contributor Author

teo1978 commented Sep 23, 2018

probably you have to add more debug info into int WiFiClient::connect()

Who's "you"? Not me, right? I'm not calling that method directly. ok I see you were talking to me; found the library files, I see I can tinker with them. Trying that out.

Anyway It definitely does look like HTTPC_ERROR_CONNECTION_REFUSED is returned whenever establishing the TCP connection fails, regardless of whether it's the connection being refused or something else.

@teo1978
Copy link
Contributor Author

teo1978 commented Sep 23, 2018

It's failing here:

int WiFiClient::connect(IPAddress ip, uint16_t port)
{
    ip_addr_t addr;
    addr.addr = ip;
    
    DEBUG_WIFI("Connecting to %s:%d\r\n", ip.toString().c_str(), port);

    if (_client)
        stop();

    // if the default interface is down, tcp_connect exits early without
    // ever calling tcp_err
    // http://lists.gnu.org/archive/html/lwip-devel/2010-05/msg00001.html
#if LWIP_VERSION_MAJOR == 1
    netif* interface = ip_route(&addr);
    if (!interface) {
        DEBUGV("no route to host\r\n");
        return 0;
    }
#endif

    tcp_pcb* pcb = tcp_new();
    if (!pcb) {
    	DEBUG_WIFI("tcp_new() failed\r\n");
        return 0;
    }

    if (_localPort > 0) {
        pcb->local_port = _localPort++;
    }

    _client = new ClientContext(pcb, nullptr, nullptr);
    _client->ref();
    _client->setTimeout(_timeout);
    int res = _client->connect(&addr, port);
    if (res == 0) {
        _client->unref();
        _client = nullptr;
        DEBUG_WIFI("ClientContext::connect() failed\r\n"); //////// <<<<<<<<<<<< HERE
        return 0;
    }

    return 1;
}

which means, ClientContext::connect() is returning zero:

// from ClientContext.h
class ClientContext
{
  // [.....]
  int connect(ip_addr_t* addr, uint16_t port)
    {
        err_t err = tcp_connect(_pcb, addr, port, &ClientContext::_s_connected);
        if (err != ERR_OK) {
            return 0; // <<<<<<<<<<<<<<<<<<<<<<<< NOTE THIS!!!
        }
        _connect_pending = 1;
        _op_start_time = millis();
        // This delay will be interrupted by esp_schedule in the connect callback
        delay(_timeout_ms);
        _connect_pending = 0;
        if (state() != ESTABLISHED) {
            abort();
            return 0; // <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< AND THIS!
        }
        return 1;
    }

So, whenever tcp_connect() returns anything other than 0, that is any error, you end up returning "connection refused".
And also (just realised, and it seems to be my case since I've found out it is not the former) when state() is not ESTABLISHED whatever that means.

@teo1978
Copy link
Contributor Author

teo1978 commented Sep 23, 2018

Note that we are actually discussing two separate issues now:

  • the original one I reported, that is return values of HTTPClient::get() other than valid HTTP response codes are undocumented (a bunch of constants in a .h files is not documentation).
  • the fact that the return value HTTPC_ERROR_CONNECTION_REFUSED is abused for a broad range of connect failures that are not necessarily related to the connection being refused.

They are both still valid, and the former has always been. I don't know why this report got closed.

@teo1978 teo1978 changed the title Undocumented return value of -1 from HTTPClient::get() Undocumented negative return values from HTTPClient::get() Sep 23, 2018
@teo1978
Copy link
Contributor Author

teo1978 commented Sep 23, 2018

Btw, I saw some DEBUGV(/*...*/) statements in ClientContext.h, and I added some. By looking for the definition of DEBUGV, I seemed to understand that its output is supposed to go to the serial port if the debug level of the Arduino IDE includes CORE (like, for example, the output of DEBUG_HTTPCLIENT() shows up if the debug level includes HTTP_CLIENT, the output of DEBUG_WIFI() shows up if the debug level includes WIFI and so on).

However, that is not the case. Is that another bug or am I missing something? I replaced the DEBUGV() statements I added with DEBUG_WIFI() and they do show up.

@devyte
Copy link
Collaborator

devyte commented Sep 23, 2018

@teo1978 this was closed because there is an issue policy in place for this repo, and because the issue template has specific instructions to follow and requests to fill out specific information, neither of which was done.
My closing comment states that if you think this issue is valid, you're invited to open a new issue and fill out the requested info.
Given the details already added here, I alternatively suggest editing your original post and replacing it with a filled out template. I will reopen and triage at that point.

@teo1978
Copy link
Contributor Author

teo1978 commented Sep 23, 2018

Well, do whatever you want.

To the part regarding lack of documentation, the template doesn't even apply. And I don't see a separate repository or bug tracker for documentation.

For the other part, maybe I'll open a separate issue, though as you say the details are all here. Either you are interested in fixing it, in which case you have all the details here, or you are not, in which case I would be wasting my time by opening a new issue and filling in the template as you request. And if you are basing your decision whether or not to fix (or triage) the issue based on whether or not I fill in a template the way you request, that in my opinion wouldn't be a sensible criteria, but of course it's up to you.

By the way, I'd suggest you to reduce the length of the template if you really expect bug reporters to read it. When I'm presented a text that long in a form I'm filling in, I'll systematically ignore it, and so will most people.

@teo1978
Copy link
Contributor Author

teo1978 commented Sep 23, 2018

Oh, I had come here to add some possibly valuable information, then I saw your comment.

In the case I'm experiencing, the failure to connect happens, as I suspected in comment #this one, here:

class ClientContext
{
  // [.....]
  int connect(ip_addr_t* addr, uint16_t port)
    {
        err_t err = tcp_connect(_pcb, addr, port, &ClientContext::_s_connected);
        if (err != ERR_OK) {
            return 0;
        }
        _connect_pending = 1;
        _op_start_time = millis();
        // This delay will be interrupted by esp_schedule in the connect callback
        delay(_timeout_ms);
        _connect_pending = 0;
        if (state() != ESTABLISHED) {
            abort();
            return 0; // <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< *** HERE ***
        }
        return 1;
    }

and the value of state(), I've found out, is 2, which if I'm not mistaken is SYN_SENT.

@Pablo2048
Copy link

This seems to be something important - please can @d-a-v take a look @ this?

@devyte
Copy link
Collaborator

devyte commented Sep 23, 2018

@teo1978 the documentation is versioned together with the code in this repo, so I don't understand why you think the template doesn't apply. Do you mean the IDE Settings and MCVE sketch? Even then, you mention specific parts of code, so specifying the core version in question is important. Or do you mean the IDE Settings and MCVE? You can, of course, specify that certain fields don't apply.

The template requests specific information to allow better triaging, searching, and planning in general. In a year or two, if this issue were to be referenced, that information would help quite a bit to get the context for when this was opened and what this issue was for. There are other reasons for requesting them, which have come from lessons learned in the past (e.g.: older issues where the template didn't exist yet).
I am not here to wrangle with you. I am here to maintain this repo, and part of that is to make sure that things are done in the way that has been agreed among the currently active developers. Filling out the template is one part of that, and it takes you maybe 3 minutes. I ask you to once again to please work with us and edit your original post and fill it out correctly, to help keep this repo in order*. If you don't understand the template, have questions about it, or need help filling it out, then please say so, and I'll help with that. It may even be that something in the template could be improved.

*When I arrived here, this repo was at 1000+ open issues and growing, and was on the verge of getting out of control, to the point that getting a new release out the door (2.4.0) was difficult in the extreme. Now it's at less than 440 open issues, and with a downward trend. Consider that right now we're only 3 active developers, all working only on our free time, plus the occasional community contributor, so getting things right to reduce workload and ease planning is important.

@devyte
Copy link
Collaborator

devyte commented Sep 23, 2018

About the above comment, I we can say that there are several things at play here:

  • Documentation needed for the HTTPC error return codes
  • Revisit/improve error handling and correct returned error codes in httpc
  • Something weird shown ClientContext where state() returns SYN_SENT (possible issue with lwip glue?)

Given the details already posted here, I suggest keeping this issue for code changes, and open a new issue for the documentation. Just please fill out the issue templates correctly.

@d-a-v
Copy link
Collaborator

d-a-v commented Sep 23, 2018

and the value of state(), I've found out, is 2, which if I'm not mistaken is SYN_SENT.

True. That means the esp did not receive the server's answer within the configured timeout which is 5 seconds by default.
So this is not a connection refused (== not connected before timeout)
This could be a server or network problem - or esp problem, does it happen often with WiFi.setSleepMode(WIFI_NONE_SLEEP) as well ?
It is at least a connection timed out (== not connected after timeout).

What code should HTTPC return in that case ?
Both HTTPC_ERROR_READ_TIMEOUT and HTTPC_ERROR_CONNECTION_LOST implies we were connected. Should we create HTTPC_ERROR_UNREACHABLE(for whatever generic today or tomorrow reason), or the more specific HTTPC_ERROR_CONNECTION_TIMEOUT ?

Do you know if there are logs on the server (it's Debian 8) where I can look, where refused connections are logged?

That would be /var/log/apache2/error.log. But you'd better tcpdump host <esp-ip> on this server to see what happens. If the server is indeed unreachable from the esp, you'll have nothing in logs. If you see the SYN, and the ACK, then you can try and increase esp's HTTPC timeout.

@teo1978
Copy link
Contributor Author

teo1978 commented Dec 15, 2018

Given the last two comments, shouldn't this be reopened?

@devyte
Copy link
Collaborator

devyte commented Dec 15, 2018

I said:

Given the details already posted here, I suggest keeping this issue for code changes, and open a new issue for the documentation. Just please fill out the issue templates correctly.

Then @d-a-v asked questions that weren't answered.

Please provide the requested info.

@teo1978
Copy link
Contributor Author

teo1978 commented Dec 15, 2018

Then @d-a-v asked questions that weren't answered.
Please provide the requested info.

I don't have the answers. It seems to me that enough information has been collected to state that there is indeed an issue to be fixed, so I think the correct status is open, whether or not I can answer to those questions.

Some of the question don't seem directed to me as the issue poster, but rather open questions as in "what should we do with xxx". The mere existence of such questions seems to me a reason for the issue to be open.

E.g.

What code should HTTPC return in that case ?

I don't know or have a suggestion for that one.

@bill-orange
Copy link

bill-orange commented Apr 22, 2019

What is the proper usage for .errorToString() I thought that this would work:

#include <HTTPClient.h>

HTTPClient http;  //Declare an object of class HTTPClient
    http.setTimeout(8000);
    const char OpenWeatherMap[] = "http://api.openweathermap.org/data/2.5/forecast?q=" OPEN_LOCATION "&units=imperial&appid=" OPEN_API;
    Serial.println(OpenWeatherMap);
    http.begin (OpenWeatherMap);
    httpCode = http.GET();
    Serial.print("Unprocessed return code: "); Serial.println (httpCode.errorToString() ); Serial.println();

What i get is: request for member 'errorToString' in 'httpCode', which is of non-class type 'int'

What am I doing wrong?

I also tried this: http.errorToString() returns the error no matching function for call to 'HTTPClient::errorToString()'

This is on an ESP32. Perhaps the ESP32 library has a bug in this non-critical function.

@d-a-v
Copy link
Collaborator

d-a-v commented Apr 22, 2019

Serial.println( http.errorToString(httpCode) ); ?

@bill-orange
Copy link

@d-a-v Yup, that appears to work. Looks weird but it works.

@teo1978
Copy link
Contributor Author

teo1978 commented Aug 7, 2019

Now I'm getting error -11 HTTPC_ERROR_READ_TIMEOUT

Nothing is logged on the server, so the HTTP request never gets through.

Here's the debug output:

connected with XXXXX, channel 1
dhcp client start...
ip:192.168.1.90,mask:255.255.255.0,gw:192.168.1.1
[HTTP-Client][begin] url: http://xxxxxxx.com/api/image/download?mac=86f3eb94e5a8
[HTTP-Client][begin] host: xxxxxxx.com port: 80 url: /api/image/download?mac=86f3eb94e5a8
[HTTP-Client] connected to xxxxxxx.com:80
[HTTP-Client] sending request header
-----
GET /api/image/download?mac=86f3eb94e5a8 HTTP/1.1
Host: xxxxxxx.com
User-Agent: ESP8266HTTPClient
Connection: close
Accept-Encoding: identity;q=1,chunked;q=0.1,*;q=0

-----
[HTTP-Client][returnError] error(-11): read Timeout
[HTTP-Client][returnError] tcp stop
[HTTP-Client] getStreamPtr: not connected

@teo1978
Copy link
Contributor Author

teo1978 commented Aug 7, 2019

Wow, it was actually timing out.
Apparently the default 5000 ms is not enough.

@teo1978
Copy link
Contributor Author

teo1978 commented Oct 6, 2019

Note that we are actually discussing two separate issues now:

  • the original one I reported, that is return values of HTTPClient::get() other than valid HTTP response codes are undocumented (a bunch of constants in a .h files is not documentation).
  • the fact that the return value HTTPC_ERROR_CONNECTION_REFUSED is abused for a broad range of connect failures that are not necessarily related to the connection being refused.

They are both still valid, and the former has always been. I don't know why this report got closed.

And to that I add:

  • the fact itself that HttpClient sometimes fails to connect in the first place, for no good reason. By now it's pretty clear there's no hardware or network issue, it's obviously randomly failing because of some bug.

See #6169 (comment)

Forget the timeout: I'm using http.setTimeout(20000), so I never get HTTPC_ERROR_READ_TIMEOUT now.

@YordanYanakiev
Copy link

Got the same issue here even with 22000 timing.

@pluisi
Copy link

pluisi commented Jul 4, 2021

Yordan did you resolve this? how?

@YordanYanakiev
Copy link

YordanYanakiev commented Jul 4, 2021

To tell You the truth it turns to be a hell of connected issues mainly and mostly related to the WiFi implementation than the HTTPClient itself.
As the issues stack on each other for me specifically was decreasing the power of the WiFi, since my board itself turns out to be underpowered on battery, then reverting to 1.0.4 SDK, and then fixing manually the issues in 1.0.4 with parts which is working from 1.0.5 and 1.0.6, but 1.0.6 turns out to be the worse with the highest connection timeout.
P.S. - I haven't tested on 2.0.0 alpha since I have compatible issues with my specific board.

@cenymaster
Copy link

Get error is:
Serial.println(HTTPClient::errorToString(httpCode));

@Akshaykushawaha
Copy link

Now I'm getting error -11 HTTPC_ERROR_READ_TIMEOUT

Nothing is logged on the server, so the HTTP request never gets through.

Hey, i think i am missing something but i feel u guys directly jumped to the point where u got error code as -11

i am getting error code as -1 for 2 days now, cannot find help anywhere, How did you resolve it?

@azizbecha
Copy link

Now I'm getting error -11 HTTPC_ERROR_READ_TIMEOUT
Nothing is logged on the server, so the HTTP request never gets through.

Hey, i think i am missing something but i feel u guys directly jumped to the point where u got error code as -11

i am getting error code as -1 for 2 days now, cannot find help anywhere, How did you resolve it?

Hey, I had the same issue. It was solved by replacing WiFiClient client with WiFiClientSecure client because WiFiClient client is used for requests sent to HTTP domains so it will return -1 when requesting HTTPS domain.

I hope it helps !

@jussitamminen
Copy link

True. That means the esp did not receive the server's answer within the configured timeout which is 5 seconds by default. So this is not a connection refused (== not connected before timeout) This could be a server #or network problem - or esp problem, does it happen often with WiFi.setSleepMode(WIFI_NONE_SLEEP) as well ? It is at least a connection timed out (== not connected after timeout).

I had the same problem as stated in the original post. I was getting -1 error as a response for HTTPClient::get(), although the link was working on a browser. I then tried this: WiFi.setSleepMode(WIFI_NONE_SLEEP) (mentioned by d-a-v), and at least for now the error is gone.

Lastly, I provide some background information on my situation. This problem occurred to me after I started using my data logging code on a new ESP8266 board. There were three changes in my system. The new board has a CP2102 chip while the old one had either CH340 or CH341 (I don't remember which one it was). I had also upgraded my wifi network to mesh wifi6 using TP-link deco x50 devices. The last change was swapping a DHT11 sensor for a better BME280 sensor. So the problems started after these three changes, and I don't know which one it caused but I wanted to share this info anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests