Skip to content
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

Honor timeout in HTTPClient #6056

Merged
merged 5 commits into from
May 6, 2019
Merged

Honor timeout in HTTPClient #6056

merged 5 commits into from
May 6, 2019

Conversation

d-a-v
Copy link
Collaborator

@d-a-v d-a-v commented May 6, 2019

fix #6033
(bug is reproduced and fixed)

@d-a-v d-a-v added this to the 2.5.1 milestone May 6, 2019
@earlephilhower
Copy link
Collaborator

I don't see where the write timeout is going to be different between the versions. The online diff seems to have lost its mind, but manually eyeballing I'm not seeing any new checks. What did I miss?

@d-a-v
Copy link
Collaborator Author

d-a-v commented May 6, 2019

What did I miss?

It's not that tricky. Timeout is implemented by Stream::readBytes() and only there.
The issue is the same in HTTPClient::writeToStreamDataBlock() and the fixed example.

readByte()'s timeout was never triggered because of the use of available() which ensured that all requested data would be readable.
The PR avoid the use of available() and ask for what's really requested. The timeout mechanism then can work.

Check the example, and this more readable subcommit.

edit: This was tested with the (now-)fixed example and a web server using netcat offering an incomplete answer in an unclosed connection.

Copy link
Collaborator

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Only minor niggle is that Stream::readBytes can return 0 for either timeout or connection loss. The handling in these cases looks good, and I suppose a dropped connection would qualify as a timeout (since you'll never get any more data). :)

if (!bytesRead)
{
DEBUG_HTTPCLIENT("[HTTP-Client][writeToStreamDataBlock] input stream timeout\n");
free(buff);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using above:

std::unique_ptr<uint8_t[]> buff(new uint8_t[buff_size]);

above instead of the malloc, and removing the three free(buff) calls

@d-a-v d-a-v merged commit e67cc90 into esp8266:master May 6, 2019
@d-a-v d-a-v deleted the httpclienttimeout branch May 6, 2019 19:26
This pull request was closed.
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

Successfully merging this pull request may close these issues.

ESP8266 hangs when using HttpClient.getString
3 participants