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

ESPhttpUpdate.update closes ESP8266WebServer connection on getting file. #3359

Closed
fulf opened this issue Jun 18, 2017 · 13 comments · Fixed by #6969
Closed

ESPhttpUpdate.update closes ESP8266WebServer connection on getting file. #3359

fulf opened this issue Jun 18, 2017 · 13 comments · Fixed by #6969

Comments

@fulf
Copy link

fulf commented Jun 18, 2017

Basic Infos

Hardware

Hardware: Wemos D1 mini Pro
Core Version: 2.3.0

Description

When the ESPhttpUpdate starts downloading the requested file, all ESP8266WebServer connections seem to close. As such, sending a message to a client stating the update was done successfully cannot be achieved.

In the example code, if the server fails supplying the file, or sends a 304 request, the result propagates to the client requesting via POST the /esp endpoint. But if the server supplies the file, the connnection is terminated, the client basically having to deal with an ERR_EMPTY_RESPONSE.

Sketch

#include <Arduino.h>
#include <ESP8266WiFi.h>
#include <ESP8266WebServer.h>
#include <ESP8266httpUpdate.h>

ESP8266WebServer _web_server;

void setup() {
   WiFi.softAP("MyESP");
   ESPhttpUpdate.rebootOnUpdate(false);
   _web_server.on("/esp", HTTP_POST, [&](){
      HTTPUpdateResult ret = ESPhttpUpdate.update(_web_server.arg("firmware"), "1.0.0");
      switch(ret) {
        case HTTP_UPDATE_FAILED:
            PREi::sendJSON(500, "Update failed.");
            break;
        case HTTP_UPDATE_NO_UPDATES:
            PREi::sendJSON(304, "Update not necessary.");
            break;
        case HTTP_UPDATE_OK:
            PREi::sendJSON(200, "Update started.");
            ESP.restart();
            break;
      }
   });
}

void loop() {
   _web_server.handleClient();
  yield();
}
@bhamon
Copy link

bhamon commented Jun 29, 2017

@fulf I also had the problem. Here is the piece of code causing your issue.

It may be for stability reason so that the update doesn't fail.

@fulf
Copy link
Author

fulf commented Jun 29, 2017

@bhamon Thank you. I seem to have missed those lines while skimming through the update code. I'm quite curious if there's any way to mitigate it and still send the client some sort of a-ok response after finishing.

Maybe a callback to be called before starting the update, so you can at least respond with a "Update started" sort of message...

@bhamon
Copy link

bhamon commented Jun 30, 2017

Seems like @igrr is the original author of this piece of code.
Can we have an explanation on this?

@manufacturist
Copy link

I'm also interested in an explanation.

@igrr
Copy link
Member

igrr commented Jun 30, 2017

Yes this is mostly for stability reasons. While OTA is in progress, application can not process incoming TCP data. If you have a web server running on the ESP, input data is processed when you call server.handleClient(). During OTA this function does not get called, so all received TCP segments will be piled up in memory, potentially making the chip run out of heap. Another important aspect is that there is a limited number of receive buffers available in the system. Even if you don't run out of memory, all buffers may be used up, at which point OTA connection will become dysfunctional, as ESP will not be able to receive any more OTA data.
Closing everything is a bit harsh, but without this step it is too easy to flood the ESP. If you need you can restart WebServer after OTA is complete.

@fulf
Copy link
Author

fulf commented Jun 30, 2017

@igrr Thanks for the reply and great explanation. That makes sense and I understand the motive behind.

Do you think we can find a middle way in which we answer with a 205 Reset Content, or (though uglier) a 503 Service Unavailable code to all open TCP requests?

@igrr
Copy link
Member

igrr commented Jun 30, 2017

In general you don't know whether any particular connection is HTTP or not. MQTT connection (which is also TCP) would require some different message to be sent, for example. Per-connection hooks are something that can be supported, but I think the easiest is to rewrite the example to perform OTA outside of the web server callback. I.e. you request the ESP to perform an update, it sets a flag that update is required and responds immediately ("update started"), then in the main loop you check the flag value and perform an update. Once the update is complete, you call .begin method for the webserver again and it can accept new connections. You can use a different endpoint to query update status, with the status stored in a variable set after update completes or fails.

@hvxl
Copy link

hvxl commented Jul 14, 2018

As you say, closing all TCP connections just because there may be some that could cause problems, is a bit harsh. Actually, it is very harsh. It makes reporting back the progress/result of the OTA process in any meaningful way pretty much impossible.

When I take out the WiFiUDP::stopAll() and WiFiClient::stopAllExcept() lines, an OTA upgrade of my application still works fine and I can report the results via a websocket.

Would it be an idea to make this part of the code conditional and do something similar to rebootOnUpdate()? In other words, have a function that a developer can call to basically indicate "I know what I'm doing, you don't have to protect me from possible dangers". Maybe call the function severConnectionsOnUpdate() with a default of true.

@ahmedezzatmtc
Copy link

please post us the complete code

@hvxl
Copy link

hvxl commented Sep 30, 2019

This is the patch I made at the time (against esp8266 version 2.4.1):
ESP8266httpUpdate-patch.txt

@iggr
Copy link

iggr commented Sep 30, 2019

Could members of this thread read and type usernames carefully.
I'm tired to receive email notifications and mute this topic numerous of times simply because my username "iggr" is very similar to "igrr" ?
Thank you.

timstableford added a commit to timstableford/Arduino that referenced this issue Feb 22, 2020
Made severing connections optional as per the patch
in the issue.
Also fixed a minor spacing issue.
earlephilhower pushed a commit that referenced this issue Feb 23, 2020
* Resolved issue #3359

Made severing connections optional as per the patch
in the issue.
Also fixed a minor spacing issue.

* Renamed sever to close and added information to readme

Also my editor automatically removed some odd whitespace at the
end of a few lines.
@timstableford
Copy link
Contributor

Good news all. 2.5 years after being reported (but only 2 months after a PR) this is finally fixed :)

@devyte
Copy link
Collaborator

devyte commented Feb 23, 2020

Closed via #6969.

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 a pull request may close this issue.

9 participants