Content length and buffer len

[vortigont]

Hi team,
@mathieucarbou, @willmmiles, @me-no-dev
let's discuss what are we gonna do with this further
As I see the root cause was actually in AsyncJson and ChunkPrint classes, other code like file or static strings response was not impacted. But the expected behavior for _fillBiffer calls is now unclear and undocumented.
Now we have a forked scenarios - for chunked responses _fillBiffer receives just the buffer and it's up to the derived class to track the amount of data it intends to write, and for non-chunked we act under assumption that contentLength is known and set and it is up to AsyncAbstractResponse to keep track of transferred data and limit buffer space. So I have two concerns:

1. the contentLength is unknown but response is NOT chunked
2. when using templates does that even makes sense to operate with contentLength at all

I do fully understand that it's an old design with all pros' and con's but templating implementation embedded in a way like today does complicate things and makes adopting design changes not that easy.

[mathieucarbou]

Hi,
I do not have a strong opinion on this one: this is SPI api so I agree for a change but we need to test correctly and document in the code the behavior change.
Also I think the parameter name would need to be changed for more clarity.

[mathieucarbou]

I am not sure we have a use case for 1) ?

If the content lengrh is not know in theory we need the corresponding chunk encoding header right ?

[vortigont]

I'm not sure if 1) is actually used in current lib classes, I guess not :) But it's a completely valid case at least for HTTP 1.0 and probably for HTTP1.1 also (need to check). If server sends reply with a body but without content-length header then it must set connection: close header for HTTP1.1 and client SHOULD read the response body till the server closes connection (i.e. a stream mode). Chunked transfer relates to encoding and is not mandatory.
I never used templates feature in AsyncWebServer but I was under the impression it should work this way. Otherwise I do not understand how it works :))) template callback could change body length significantly and proper content length value is impossible to set in advance.
So there is definitely a design gap.

[mathieucarbou]

OK! I thought the template code was using the chunk encoding. I didn't know we could stream bytes without content length or chunks.

On another note: if you look at this discussion here:

emsesp/EMS-ESP32#2752 (comment)

This is another (big) project where they also caught the recently fixed bug. They are usually testing carefully resource usage and they found a heap usage increase which I suspect comes from them doing a load test on some handlers but we now have an added buffer of 3K per response.

So something that is bothering me is this code:

ESPAsyncWebServer/src/WebResponses.cpp

Lines 438 to 446 in b170b9d

	if (!_send_buffer) {
	auto p = new (std::nothrow) std::array<uint8_t, ASYNC_RESPONCE_BUFF_SIZE>;
	if (p) {
	_send_buffer.reset(p);
	_send_buffer_len = _send_buffer_offset = 0;
	} else {
	break; // OOM
	}
	}

If we cannot allocate, shouldn't we, instead:

• do a asyncws_log_e() to log the error
• call client->abort()
• return to exist the function asap

?

[me-no-dev]

I would say, since we are the server, lets enforce either content length or chunked if not provided. While missing content length might be a valid thing (long time ago), there is no point in supporting it anymore. It will only complicate things and could cause issues if the receiver does not expect it. As far as I remember, I never intended to support missing content length outside of chunked responses.

[vortigont]

checked the code, templated response does use chunked indeed, so should be OK.
While totally agree on recommended scenario to use chunked encoding for responses with unknown len we can't enforce users to do same providing them base class to implement their own tricks if any. I quickly checked existing classes and looks like AsyncResponseStream uses _contentLength = 0 and not uses chunked encoding (though I've never tried to use it and not sure how it operates if writing from different threads). So the case might still be valid.
Since current server code does not implement HTTP1.1 pipe-lining we do enforce connection: close anyway.
Now taking current code std::min(std::min(_send_buffer->size(), tcp_win), _contentLength - _sentLength)
looks like it'll still work if _contentLength == 0, it will just overflow the subtract to some huge unsigned value but as for me this behavior is quite cryptic to find and understand. It needs some documentation at least.

If we cannot allocate, shouldn't we, instead:

no, the idea is that if alloc fails then this method would just break and exit, then it might retry later on next tcp_recv on poll, so there is no need to abort the connection abnormally.

[mathieucarbou]

it will just overflow the subtract to some huge unsigned value but as for me this behavior is quite cryptic to find and understand. It needs some documentation at least.

Indeed!

no, the idea is that if alloc fails then this method would just break and exit, then it might retry later on next tcp_recv on poll, so there is no need to abort the connection abnormally.

Yes, but that's exactly the behavior that I wanted to discuss... Is it normal to break and retry if the MCU lacks heap memory ? Shouldn't we instead abort the request to help free memory for potentially other things running in // ? Because if retrying, same thing will happen again and again if we do not "force" a hea memory decrease ?

[willmmiles]

Because if retrying, same thing will happen again and again if we do not "force" a hea memory decrease ?

Not necessarily, that depends a lot on the application. It could be in the middle of some expensive but transient operation - say, sending a large JSON object to an MQTT server. The web server has no way to know; hard aborting requests would be somewhat unfriendly to those users, create intermittent web interface failures that are hard to reproduce or debug.

At the very least, if we keep retrying and failing, at some point the connection should time out, which releases the heap used by that connection (though it can take a while!).

That said, a common case in my low heap testing was that the memory was being used by other web server requests in flight. If there's enough RAM that even one of them can proceed, eventually it'll clear out, leaving heap available to service other responses. But under memory pressure, it was pretty easy to livelock the server under load, though again it will eventually - over minutes - time out and clear. (Hence the major feature in the WLED fork being a request queue based on heap usage estimates.)

From an implementation standpoint, dealing with this was actually the major reason I went with a variable size assembly buffer in that fork - it lets the code fall back to smaller buffers and still make some progress even if the heap becomes too fragmented to allocate a full size one. It's really too bad we can't ask the TCP stack to give us the output packet buffer directly...