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

Collection of fixes for WLED project #11

Merged
merged 58 commits into from
Mar 10, 2024
Merged

Conversation

willmmiles
Copy link
Collaborator

As referenced in Aircoookie/WLED#3784 , this is the collection of upstream PRs and bugfixes I've been working on to try and improve performance and stability, particulary for ESP8266es.

Highlights include:

  • Merging several upstream PRs
  • Fix for use-after-free in web server response handling
  • Fix for memory corruption in web sockets
  • Replace web socket custom memory management with std::shared_ptr, fixing memory leaks
  • Move a lot of static strings to PROGMEM
  • Handling of partial buffer TCP writes under low memory conditions

me-no-dev and others added 30 commits October 2, 2019 10:32
remove extra #endif that should have been dropped as part of me-no-dev@59066bd
* Added method to access clients of AsyncWebSocket

* me-no-dev#571

* Conflict with crypto library
* Fix compile warnings

* first test

* Add regex path

* Add simple example

* add support for esp8266

* Update AsyncJson.h
…CONNECT (me-no-dev#559)

Signed-off-by: Arthur Frederico Neves <arthurfred.neves@gmail.com>
* Add methods to determine Average Queue length
* Change compiler error to runtime error.
Add compiler warining when a ASYNCWEBSERVER_REGEX function is used, but not enabled

* Update docs for usage in Arduino IDE

* Update docs for platform.local.txt
…wing messageQueue (me-no-dev#621)

* Prevent tcp/wifi DOS lockup by preventing number of messages in queue, drop otherwise

* Define (renamed) MAX_SSE_Clients
Changes to be committed:
modified:   src/WebHandlerImpl.h
…e-no-dev#999)

* Fix build when using latest arduino-esp32 master due to IDF update
espressif/arduino-esp32@a618fc1

* Fix build when using WebSockets
Upgrade the list structure to support quick insertion, and fix a
use-after-free in _removeNotInterestingHeaders.  This bug has been
reported and "fixed" many times upstream, but several of those fixes
seem to be bad.

See me-no-dev/ESPAsyncWebServer me-no-dev#951, me-no-dev#837
This reduces the total number of allocations required for every request.
Also includes a pre-allocation optimization to
AsyncWebServerResponse::_assembleHead and some more migration to
PROGMEM strings.
Allow the packet buffer to persist, so we can support partial writes in
the event that the entire packet cannot be written at once.

Includes some heuristics on ESP8266es to ensure that there's enough RAM
for the heap and outgoing packet buffers.
This reduces the number of allocations and copies required.
This speeds up our case as we don't need to zero the memory first.
In a misguided attempt to work around badly written clients,
AsyncWebSocketClient::_onData was trying to restore the value of the
byte one past the end of the packet buffer.  Unfortunately, that value
might have been *correctly* changed by the callback, such as when it's
storing heap metadata.

This seemed to be causing of websocket-related crashes in WLED on
ESP8266.
Move a few long strings into PROGMEM.
There sure are a lot of long fixed strings.
Also lets us accept PROGMEM for function inputs.
Replace the hand-rolled buffer management logic with an RAII buffer
class based on std::shared_ptr.  This simplifies memory management and
seems to fix a memory leak, while also offering a clean move semantic
for clients.

The previous API is preserved through a compatibility object; it is
flagged as deprecated as the old API required a specialized allocator,
which is now unneccessary, so for compatibility reasons an extra heap
allocation is used.
@willmmiles
Copy link
Collaborator Author

Embarrassingly, also included is a metadata patch changing the name to 'ESPAsyncWebServerWLED', updating the version number to 2.1, and listing myself as maintainer. Sorry for the fork, I wasn't sure if you'd have an opportunity to review this work anytime soon. Would you like me to revert those changes?

@blazoncek
Copy link
Collaborator

Even though I do not understand half of it I do understand that you did an exceptional work.

Without any change in WLED code this frees about 5kB of RAM on ESP8266 alone which is outstanding in itself.
So far my tests on ESP8266, ESP32, ESP32-S2 and ESP32-C3 are nothing but flawless.

I am curious, though, why are all those shell scripts needed and how do we change those deprecated methods like ws.makeBuffer()?

@Aircoookie if I understand correctly, your fork was intended primarily to prevent serving wsec.json as it contains passwords and such. As I know this has to remain within WLED control I urge you to merge these changes ASAP as the benefits are enormous (instead of changing dependency library in WLED). If I may, I would also recommend to grant @willmmiles collaborator rights on this repository.

@willmmiles
Copy link
Collaborator Author

I am curious, though, why are all those shell scripts needed

I merged the changes from the upstream web server repo, which had a lot of CI script work done since the fork. I didn't bother filtering their commits, it seemed to make more sense to be a strictly downstream version.

Sadly those scripts have also bit-rotted and no longer work. I'd looked at them briefly - I believe me-no-dev is or was employed by espressif themselves, and some of the espressif repos have newer versions of those scripts -- but they haven't been my priority.

and how do we change those deprecated methods like ws.makeBuffer()?

I've got a patch for WLED to eliminate the extra allocation and indirection already in my fork, though it needs to be updated to the final object model. Once the web server changes are accepted (one way or the other) I'll send a PR there.

@Aircoookie Aircoookie merged commit c0fbcc2 into Aircoookie:master Mar 10, 2024
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.