Update AsyncTCP (and AsyncWebServer) #4796
Conversation
This should fix (or at least improve) some of the crash cases under excessive web server load.
WalkthroughThe PlatformIO configuration file was updated to specify newer versions and sources for the ESPAsyncWebServer and AsyncTCP libraries. The ESPAsyncWebServer dependency was bumped to v2.4.2, and the AsyncTCP dependency for the esp32_all_variants environment was switched to a different repository and updated to v3.4.7. Additionally, a new build flag was added for the esp32_all_variants environment. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
platformio.ini(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: blazoncek
PR: wled/WLED#4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
platformio.ini (1)
Learnt from: blazoncek
PR: #4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32dev_V4)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
🔇 Additional comments (1)
platformio.ini (1)
145-145: Inconsistent ESPAsyncWebServer version pinning – please confirm it’s intentionalThe global
lib_depsblock (platformio.ini line 145) now pins ESPAsyncWebServer to v2.4.1, while the compat environment (platformio.ini line 233) still references v2.4.0. This split could introduce subtle ABI/feature mismatches at link-time if it wasn’t deliberate.• platformio.ini:145 → https://github.com/Aircoookie/ESPAsyncWebServer.git#v2.4.1
• platformio.ini:233 → https://github.com/Aircoookie/ESPAsyncWebServer.git#v2.4.0If you intended to upgrade all environments, please align both pins to the same version.
|
When we are happy this is good to merge, should it also get merged to 0_15_x ? |
Depends on how long that takes ;) Honestly I'd recommend moving quickly on this one; feedback on AsyncTCP 3.4.6 has been extremely encouraging. |
|
btw. the lockup I saw in the UI were, as you predicted, purely heap related. With my latest fixes on that PR it is very stable. Would not call it rock solid yet but maybe this update to AsyncTCP will make it so. |
It turned out that there were two bugs in AsyncTCP: a use-after-free if a connection was dropped due to overload at the TCP layer, and a race condition that could result in a double-free if the remote end closed the connection just after sending data completed. In both cases the result was memory corruption that typically manifested as crashes in heap functions, as heap data structures (and guard segments) were getting overwritten. |
Bugfix on 3.4.6
|
Updated after another AsyncTCP bug fix (memory leak handling aborted connections such as Firefox attempting SSL even when given an http:// URL) |
We downtuned the stack usage of AsyncTCP, and at some point in the history of our fork, this got folded in to the default. Re-apply the stack size we've been using and recover that RAM.
Update to the latest upstream AsyncTCP library, which now includes most of the improvements from the willmmiles fork, and specifically includes a fix for the crashes reported at #4785. (It is my intention to PR the remaining improvements from my fork there anyways; the additional scrutiny has been good for turning up bugs.)
Unfortunately, AsyncWebServer also must be updated with the same reference: if it points to a different library source, the PlatformIO build process can get quite confused.
Summary by CodeRabbit