- Notifications
You must be signed in to change notification settings - Fork13.3k
Avoid large reallocations in webserver's header response#8873
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
CI seems to be aborting (not code related) frequently right about now. Let's try to re-run later... |
| @@ -420,7 +420,8 @@ void ESP8266WebServerTemplate<ServerType>::stop() { | |||
| } | |||
| template <typename ServerType> | |||
| void ESP8266WebServerTemplate<ServerType>::sendHeader(String&& name, String&& value, bool first) { | |||
| template <typename S1, typename S2> | |||
| void ESP8266WebServerTemplate<ServerType>::sendHeader(S1 name, S2 value, bool first) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
minor nit
at a glance, we have a bunch of typed pairs that generate multiple emplace_... for every sendHeader variant
I'd consider using perfect forward and a specific type we already have
using Pair = std::pair<String, String>;autofoo(Pair pair,bool condition) {if (condition) {foo(std::move(pair)); }else {bar(std::move(pair)); }}template<typename S1,typename S2>autofoo(S1&& name, S2&& value,bool condition) {foo(std::make_pair(String(std::forward<S1>(name)),String(std::forward<S2>(value)), condition);}
@d-a-v I have pulled your branch and did a quick test. Looks good. Thanks for the quick fix! What I found when more that one client is connecting to the WebServer is this: It's surely not releated to this change and I even do not know if it comes from ESPWebServer or our code ... |
| for (const auto& kv: _userHeaders) | ||
| if (!_streamHeader(kv.first, kv.second)) | ||
| return false; | ||
| _userHeaders.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
When will these be cleared if streaming failed?
N.B. I got the impression the current implementation of the webserver does hold some data way longer than needed as opening a web page may sometimes free up quite a bit of memory. (maybe POST data of a previous request???)
I think the same happens in theESP8266HTTPClient.cpp#addHeader ... if (first) { _headers = headerLine + _headers; }else { _headers += headerLine; } No one knows in advance how big |
No description provided.