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

Bugfix/esp8266 http client #5250

Merged
merged 14 commits into from
Oct 21, 2018
Merged

Bugfix/esp8266 http client #5250

merged 14 commits into from
Oct 21, 2018

Conversation

Jeroen88
Copy link
Contributor

Issue #5216 was incorrectly solved with PR #5232. This PR corrects this. In PR #5232 a Connection: Keep-Alive was not handled: a connection was always closed. Moreover, the cause of the crash of BasicHttpsClient.ino and StreamHttpsClient.ino did not lie in the call to _client->stop() from the destructor ~HTTPClient as assumed in PR #5232, but because the HTTPClient object was destructed after the WiFiClientSecure object was destructed and ~HTTPClient was still referencing WiFiClientSecure.

In this PR:

  • Connection: Keep-Alive is handled correctly
  • The examples BasicHttpsClient.ino and StreamHttpsClient.ino are changed. The HTTPClient is put into a seperate scope block to ensure it is destructed before WiFiClientSecure is destroyed
  • Modified checks are added to all begin() methods to check for a mixed use of deprecated begin() calls and the new begin() calls. Of course mixing up both begin()'s is discouraged, as is the use of the deprecated functions
  • An unnecessary check in ::disconnect() is removed

@Jeroen88
Copy link
Contributor Author

@earlephilhower Hi Earle, did you see this PR?

if (httpCode == HTTP_CODE_OK || httpCode == HTTP_CODE_MOVED_PERMANENTLY) {
String payload = https.getString();
Serial.println(payload);
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is technically correct: the client object must outlive the http object that uses it.
However, it is rather unclear at first glance that this is the reason for the scope. Also, someone looking at the example would be unlikely to realize the importance of the scope.
Instead, consider wrapping the allocation in a std::unique_ptr:

std::unique_ptr<BearSSL::WiFiClientSecure> client(new BearSSL::WiFiClientSecure);

Then, the internal object can be passed by get():

if (https.begin(*client, "https://jigsaw.w3.org/HTTP/connection.html")) {  // HTTPS
...

Because both objects are in the same scope, and because the client must be declared before the http object, when exiting the scope their destruction will automatically happen in the correct order: inverse of their construction.
Also, no need to call delete.
Also, I don't like having manual handling of dynamic objects in an example ino, which will be viewed and used by inexperienced users, so smart pointers is better.

@@ -55,61 +53,69 @@ void loop() {
const uint8_t fingerprint[20] = {0xEB, 0xD9, 0xDF, 0x37, 0xC2, 0xCC, 0x84, 0x89, 0x00, 0xA0, 0x58, 0x52, 0x24, 0x04, 0xE4, 0x37, 0x3E, 0x2B, 0xF1, 0x41};
client->setFingerprint(fingerprint);

if (http.begin(*client, "https://tls.mbed.org/")) {
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above: consider wrapping in a std::unique_ptr, and removing the scope.

@Jeroen88
Copy link
Contributor Author

Jeroen88 commented Oct 18, 2018

@DEVTE Thanks for the advice to use a unique_ptr. I updated both examples BasicHttpsClient.ino and StreamHttpsClient.ino.
In the latter I removed the WiFiClient * stream = client read directly from the client instead, because the casting of the pointer did not work anymore. Could you help me with how to properly cast the unique_ptr to the base class?
By the way, both examples fail to work now. I am searching for hours now, and I do not think it has anything to do with ESP8266HTTPClient nor with boths .ino examples. Did anything else change?

@Jeroen88
Copy link
Contributor Author

The examples work again without any changes in the HTTP client library, most likely due to a fix in other libraries. The latest commit is just merging the latest master into this PR.

@devyte devyte merged commit e954022 into esp8266:master Oct 21, 2018
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.

2 participants