Request: Add success/fail flag for webserver parse arguments

[TD-er]

On my node (running the latest core version as of yesterday), I have really low free RAM, but this may also happen on nodes with more fragmented memory or for long POST requests.

What I'm seeing is that when submitting form data, not all form elements will be complete.
As far as I know there is no easy check to see if the web arg parsing was successful.

ESP8266WebServerTemplate<ServerType>::_parseArgumentsPrivate does seem to parse the form data and create an array of _currentArgs containing a key and value string.
So far so good, but sometimes I am missing a key or its value is incomplete.

The storeArgHandler handler given to this parser function would be the ideal place to check if a result was successful, since it does know how long the strings should be when stored.

The ESP8266WebServerTemplate<ServerType>::urlDecode function should be changed to not return a String, but filling one and also be responsible of calling reserve as soon as it knows how long the string will be. (will cause less heap fragmentation also, but may need to check the string twice)
This will make it quite clear very soon if an alloc (after calling the reserve()) was successful.
If not, then there is no use to continue.
So it does also make sense to return a bool.

This makes it also clear void operator() (String& key, String& value, const String& data, int equal_index, int pos, int key_end_pos, int next_index) should return a bool.

ESP8266WebServerTemplate<ServerType>::_parseArgumentsPrivate should then return -1 on an error and its result should be dealt with here:

template <typename ServerType>
void ESP8266WebServerTemplate<ServerType>::_parseArguments(const String& data) {
  if (_currentArgs)
    delete[] _currentArgs;

  _currentArgCount = _parseArgumentsPrivate(data, nullArgHandler());

  // allocate one more, this is needed because {"plain": plainBuf} is always added
  _currentArgs = new RequestArgument[_currentArgCount + 1];

  (void)_parseArgumentsPrivate(data, storeArgHandler<ServerType>());
}

While we're at it, I also noticed the _parseArgumentsPrivate function is also called with a nullArgHandler, to determine the number of arguments.
It calls this handler using this code:

      RequestArgument& arg = _currentArgs[arg_total];
      handler(arg.key, arg.value, data, equal_index, pos, key_end_pos, next_index);

But _currentArgs is deleted (though not set to nullptr).
Can this cause a crash when the former address for _currentArgs is been re-used for something else and we're addressing it like this?

I do get the use of a nullArgHandler, but wouldn't it make more sense to use it as a pointer which can be null? Then we can test for it and not address an array which has just been deleted.

A last point I was wondering.
Is it needed to store the plain form element, which does contain the same information as was submitted?
If so, then wouldn't it make more sense to just store the offsets in this plain string (& decoded length) and decode it again as soon as an element is requested?
Now the data is stored twice in memory.

[TD-er]

I've been looking at the code a bit more and also came up with some other use cases which may explain a number of issues I've seen.

The arguments which are parsed are kept in an array, which is deleted as soon as the next request comes in.
This has 2 rather serious implications:

• No guarantee the data you're reading is valid (or even allocated) as soon as you try to process it.
• As long as no new http request comes in, the arguments keep on using memory.

In the webserver, you have to register some URLs and their function to handle the URL.
I guess it would make sense to move the parsed arguments to some area so they will not be destructed before the call to the set handler function is finished.
Also the handler should be the only one reading those arguments and the argument strings should be deleted as soon as the function finishes.
I think moving the arguments isn't that hard to implement, but I'm not sure yet how to make sure the handler function can access these without change of function signature.
Otherwise the webserver should create a queue for handling the requests, but I guess that could lead to a lot of RAM usage very soon. Or the webserver should respond with something like a 504 or some more appropriate reply to tell the client to retry at a later time.

As it is implemented right now is not the correct way as it does happen sometimes that values of one request are mixed with other values, or some other processes allocate memory where there was once a parsed argument.

[TD-er]

Does anyone have a comment on my suggestions?
The longer I think about it, I am more confident it is actually a bug and not really a feature req. for a flag to detect when the bug occurs.

[d-a-v]

@TD-er the webserver internals are a heavy legacy to carry.
It received many fixes but as you notice the internal are not very optimized. Specially regarding memory usage. On top of that the arg("plain") I had to add is underefficient (but fixes a bug or adds a feature - yes data are stored twice in memory, this is also referred by another issue).

@darkain had fully rewritten it and I then tried to bring it here but I am now late behind recent updates (templatization) #5636.

What can be done

• continue to debug and optimize our legacy Webserver

  but I'm not sure yet how to make sure the handler function can access these without change of function signature.

  That's the whole issue to deal with: keep compatibility

• try darkain's new version (different namespace) after I can finish the templatization, so one can test it against current code using the webserver

Otherwise the webserver should create a queue for handling the requests, but I guess that could lead to a lot of RAM usage very soon

I think the AsyncHttpServer approach is better in term of memory than trying to do that.

[TD-er]

I think the AsyncHttpServer approach is better in term of memory than trying to do that.

I was afraid you were going to say something like that.
Switching to AsyncHttpServer is not something I will be doing with the current ESPEasy web interface.
But for the new UI we're working on, it might be good to look at this change also.

So then we're kinda back at the original idea of having a flag to detect whether things were not parsed.

[d-a-v]

For the same reasons we cannot get rid of the current Webserver.
We are condemned to maintain it up (or we are not maintainers anymore :)

to detect whether things were not parsed.

Please allow time for that (if you want us to check this out).
I expect to update the other implementation soon.