esp8266 / Arduino

ESP8266 core for Arduino
GNU Lesser General Public License v2.1
16.09k stars 13.32k forks source link

ESP8266WebServer memory exhaustion while processing requests #4823

Open darkain opened 6 years ago

darkain commented 6 years ago

Basic Infos

Platform

Settings in IDE

Problem Description

With the ESP8266WebServer, while processing requests, the String library is used heavily. The way that it is being used repeatedly duplicates data while processing it resulting in memory exhaustion. I'm trying to pass in some data to the web server via POST request, and would constantly get 0 args available. After debugging for a few hours, it turned out that there was no RAM left due to these String copies.

Example 1: ESP8266WebServer::urlDecode This method reads in the source String character by character, and copies the data to a new String, appending to the end. Assuming there is nothing to decode (worst case scenario), this doubles the memory requirements for the particular argument at this stage of processing. https://github.com/esp8266/Arduino/blob/5328a8b91ef04aa10cb9cb272ba1ade5299cb810/libraries/ESP8266WebServer/src/Parsing.cpp#L580

Example 2: ESP8266WebServer::_parseRequest Pretty much the same as above, but on a larger scale. Rather than parsing URL parameters and POST data separately, they are concatenated together. This duplicates the entirety of POST data in RAM. https://github.com/esp8266/Arduino/blob/5328a8b91ef04aa10cb9cb272ba1ade5299cb810/libraries/ESP8266WebServer/src/Parsing.cpp#L195

Example 3: substring To separate out each argument, substring is used, duplicating each argument for processing. https://github.com/esp8266/Arduino/blob/5328a8b91ef04aa10cb9cb272ba1ade5299cb810/libraries/ESP8266WebServer/src/Parsing.cpp#L325

There are countless more examples, too. All in all, passing in ~1KiB of data exhausted the ~14KiB of RAM free on the board while my application was running. ALL of these could be alleviated by doing inline processing on a single buffer, doing things C style rather than C++ style for the bulk of the internal processing. This would not only greatly reduce the overall memory footprint while processing the request, it would greatly speed it up too (less memory copying, less memory processing, and only 1 buffer that would need to be allocated/freed per request)

For my particular project, I've already started working on this conversion process. If there is desire, I can make this publicly available when I'm certain it is stable.

Also, I believe this could be the cause of countless other issues I've seen reported with the ESP8266WebServer. There are absolutely no errors/warnings whatsoever when this particular memory exhaustion happens, just the appearance of an empty request (or partiality request sometimes, depending on environment status).

everslick commented 6 years ago

I think this issue plays in the same ballpark.

JonHyeKnudsen commented 6 years ago

NOT RELEVANT TO THIS ISSUE

See edits of this post if you are curious anyway

devyte commented 6 years ago

@JonHyeKnudsen completely different issue. Release 2.4.1 has known problems, including a mem leak related to sockets. Use latest git.

JonHyeKnudsen commented 6 years ago

Sorry. I downloaded latest git just hours ago and tried to run the code again. It stille shows the same problem. The Heap is steadily decreasing by about 50 each minute. Am I overlooking something that I should be releasing in my request handling seen in the above post or am I not actually using the latest git (how to I verify that it's actually being used when I flash the board?) I am using Arduino IDE on Mac OS

everslick commented 6 years ago

Your code is very hard to read, because you did not use proper MARKDOWN syntax for code snippets (three ticks before and after the source code). Consider editing your comment.

JonHyeKnudsen commented 6 years ago

Ok. Was wondering why it wasn’t marked properly as code. I had just used the “Mark as code” button, but that hadn’t marked it as intended. I edited my post so it’s more readable now.

everslick commented 6 years ago

I was looking over the code and could not spot any obvious errors or memleaks. Here is what I would do:

JonHyeKnudsen commented 6 years ago

STILL NOT RELEVANT FOR THIS ISSUE See edit of this post if you are curious.

devyte commented 6 years ago

That leak was present in 2.4.1 and has since been fixed. When you installed git, did you uninstall releAease. 2.4.1 via board manager? There is a method in the EspClass (instantiated globally as ESP) that tells you version info.

JonHyeKnudsen commented 6 years ago

NOT RELEVANT FOR THIS ISSUE See edits of this post if you are curious

JonHyeKnudsen commented 6 years ago

So I already have news. The leak does not seem to have anything to do with my actual problems, where the sketch crashes. It is still related to but the heap is not filled before it crashes.

WiFiClient client = server.available();
  if (client)

I uploaded the sketch to my "production" board and have had it running for a couple of hours. It worked flawlessly without the server running, but now with the server reenabled it just crashes after a while.

I need to figure out how I get a crash-report/stacktrace or something without the board being connected over serial to my computer. The only thing I ruled out now is that it is hardware related (as it works with a slightly simpler sketch and same hardware setup) and its not related to the heap running full.

devyte commented 6 years ago

@JonHyeKnudsen still a different issue than reported here. This issue is meant to cover inefficienct use of mem due to buffer duplication.

yoursunny commented 6 years ago

In system programming there's always a trade-off between ease-of-use and performance. The ESP8266WebServer library seems to prefer ease-of-use, so that they use a lot of Strings. One could make an HTTP server library that does not need use any String and dynamic memory allocation, but then the user would have to manually take care of memory allocation, copying, and deallocation. So the question is: do we expect hobbyists to understand those obscure concepts, or do we want to provide something easy to use?

darkain commented 6 years ago

The code in question is all internal code, not the external API the web server presents to the developer.

d-a-v commented 6 years ago

For my particular project, I've already started working on this conversion process. If there is desire, I can make this publicly available when I'm certain it is stable.

Your initial analysis is relevant. Any fix and improvements are welcome, particularly with the WebServer.

darkain commented 6 years ago

This is BY NO MEANS AT ALL complete, or even stable, you've officially been warned!

https://github.com/cosplaylighting/Arduino/tree/master/libraries/ESP8266WebServer/src

This includes my almost entire re-write of the web server. TONS of things are still missing or broken. RAM usage is significantly lower though, both at idle and while processing requests.

The idea is to use only one single memory buffer for the entire web request, and parsing its content in-place in RAM. URL/Form decoding is also done entirely in-place without allocating any new RAM either.

In the event that the single buffer allocation fails, rather than performing undefined behaviors, this scenario is now properly caught and presents the appropriate error message back to the client. This should greatly help other users who have experienced "unexplained" issues with memory exhaustion.

Before continuing to re-add all of the POST and other features, I'm working on a massive code clean up for readability.

At this point, I cannot guarantee any API compatibility with the existing ESP8266WebServer. Several methods have been changed around from (String) to (char*), however there is a shim layer to handle most of these. Due to this layer still using (String), there is no buffer allocation safety at that stage, so it could still potentially provide issues for applications that are hovering right around 100% RAM usage.

d-a-v commented 6 years ago

This is a huge and interesting work you're doing here. One of the easiest way for others to follow, discuss and test your code is by:

devyte commented 6 years ago

@darkain I agree with @d-a-v . Your initiative about this is very good, and I'd like to see you follow through with it. I strongly encourage you to make a PR with your implementation so that we can review and discuss it. There are some details about contributing that are easier to follow and fix on the fly when discussing in a PR.

darkain commented 6 years ago

Sure thing. Is there an official style guide document somewhere that I could follow? Also, I'll be out of town for a while and won't be able to get back to this for another week or two. But yes, I'd like to continue to work on this so hopefully others can benefit from it as well. For the time being, it is working in my particular application where I've been able to stress test it at several hundred requests per minute.

However, an issue outside of this library has arisen as well and I didn't get any responses from IRC when asking there. Can someone point me in the right direction as to why WiFi access point mode behaves entirely unstable whereas station mode runs perfectly? When running in AP mode, the entire WiFi stack would randomly entirey stop working until the device was rebooted (but the rest of the application continued to work without networking). This was the last big stability hangup in my application and didn't have time to investigate if it was an issue with the web server itself, the TCP stack, the underlaying MAC/WiFi stack. I was going to do some wireshark dumps, but as just mentioned, out of town for a while so that is on hold. Just hoping to have a solid starting place for when I return.

darkain commented 6 years ago

FYI: I'm still waiting on information about proper style guides before proceeding further. Plus the issue with AP mode still makes me feel uncomfortable releasing ANYTHING until I know what that issue is. It has been several months with no response at all, here or on IRC.

devyte commented 6 years ago

@darkain the styling rules aren't documented yet, mostly because it would be premature. At this time, they're being enforced only recently, only in part, and experimentally at that. There are two sets:

  1. Example sketches: rules follow "Arduino style" indent, with some minor exceptions that are considered bad practice. These are enforced here I think, by astyle.
  2. Lib and core code: rules follow mostly Allman indent for maintainability and mutual agreement of the current devs. I believe these are not enforced yet. Beyond the above, standard C++ coding style and guidelines should be observed as much as reasonable.

I believe @d-a-v was referring to point 1 above, because CI will fail if example sketches don't pass the astyle check, so anyone making a PR with examples needs to be aware of that.

d-a-v commented 5 years ago

@darkain Do you still wish to share your redesigned webserver ?

darkain commented 5 years ago

I'm uncomfortable releasing anything until the underlaying Wifi stack is fixed. I mean, my github fork is public, but I'm not doing any more work on it until Wifi is stable. I've posted the issue already here, but it has been mostly dismissed. The watchdog timer resets the chip arbitrarily anywhere between ~3 minutes and ~3 hours. There isn't much worth in my time investing into perfecting the web server itself until the tools it relies upon are fixed. I do understand that the main Wifi driver itself is closed source upstream, which is a major problem. There was already at least one major stability fix pushed to it in the past year, but there are more issues that need resolved which I personally cannot contribute to or even diagnose without that closed source code. We're basically at a stalemate.

CurlyMoo commented 3 years ago

After struggling with the same issue i've decided to write my own webserver for the ESP8266. It can run in Async and Synced mode without recompiling and it uses a stable amount of memory (without leaking). This means that initial memory is a little bit more, but it hardly requires any additional memory while running. And it solves many of the issues stated by @darkain in his first post. It won't be perfect, but solved the instability we where accounting in a project.

In line with the rewrite by @darkain it analyzes the requests in place by e.g. using memmove to make sure no additional memory is required by buffering and such. It does make the webserver less user friendly.

https://github.com/CurlyMoo/webserver/