net: http_server: Fix capturing of request headers on concurrent HTTP2 streams #82444
Conversation
zephyrbot
added
area: Sockets
zephyrbot
requested review from
fabiobaltieri,
kartben,
nashif,
pdgendt,
ssharks and
tbursztyka
There was a problem hiding this comment.
Looks good, thank you for the contribution. Note, that we now have another example utilizing HTTP server API (https://github.com/zephyrproject-rtos/zephyr/blob/main/samples/net/prometheus), need to align it with the API changes too.
mrodgers-witekio
force-pushed
the
http2_request_headers_concurrent_streams
branch
from
e3e15d9
to
682dcce
Compare
|
Rebased and updated prometheus sample |
To correctly handle concurrent HTTP POST requests via different http2 streams on the same client context, it is necessary to store the resource detail at an HTTP2 stream level rather than at an HTTP client level, otherwise only one resource detail can be stored. Signed-off-by: Matt Rodgers <[email protected]>
mrodgers-witekio
force-pushed
the
http2_request_headers_concurrent_streams
branch
from
682dcce
to
b3b6981
Compare
|
Rebased again as I noticed some of the test changes were included in the wrong commit (so tests would have failed on first commit) |
include/zephyr/net/http/server.h
Outdated
| uint8_t *data; /** HTTP request data */ | ||
| size_t data_len; /** Length of HTTP request data */ | ||
| struct http_header *headers; /** Array of HTTP request headers */ | ||
| size_t header_count; /** Array length of HTTP request headers */ | ||
| enum http_header_status headers_status; /** Status of HTTP request headers */ |
There was a problem hiding this comment.
Not sure about doxygen comments, but do we need to set them /**< ..... */ here (as they are in the same line as what is being documented)?
There was a problem hiding this comment.
Good point, I just copied the comment formatting from the struct http_response_ctx but it seems this is also wrong, and has comments associated with the wrong fields in the online docs.
Will update to fix both of these.
There was a problem hiding this comment.
it's correct this way :) it's the syntax for when the comment is put after the element it applies to, i.e. useful when one wants to make everything fit on a single line
https://www.doxygen.nl/manual/docblocks.html#memberdoc
There was a problem hiding this comment.
oops, sorry, looks like I'm blind today. @jukkar's suggestion IS the correct way :) sorry for the noise
There was a problem hiding this comment.
Yeah I just had a look at the web documentation and got this:
Have just updated with a fix
Concurrent HTTP POST requests on different HTTP2 concurrent streams require that the client's header_capture_context is re-used to capture headers on a second stream before all of the body data has been received (and sent to the application) on the first stream. As a result, any captured headers must be sent to the application callback before any headers can be received on a different stream. In practice this means that for HTTP2 the application callback is called for the first time on receiving a headers frame, before any data frames are received. All subsequent application callbacks will not include the request header data. While this mechanism is not necessary for HTTP1, it is also updated to only send headers in the first application callback for consistency. Fixes zephyrproject-rtos#82273 Signed-off-by: Matt Rodgers <[email protected]>
Dynamic resource callback function signature has changed to combine the data, data length and request header details into a struct http_request_ctx. Update documentation to describe this, and add a note in the 4.1 migration guide since this is a public API change. Signed-off-by: Matt Rodgers <[email protected]>
mrodgers-witekio
force-pushed
the
http2_request_headers_concurrent_streams
branch
from
b3b6981
to
d531324
Compare
Fixes #82273.
Concurrent HTTP POST requests on different HTTP2 concurrent streams require that the client's header_capture_context is re-used to capture headers on a second stream before all of the body data has been received (and sent to the application) on the first stream.
As a result, any captured headers must be sent to the application callback before any headers can be received on a different stream. In practice this means that for HTTP2 the application callback is called for the first time on receiving a headers frame, before any data frames are received. All subsequent application callbacks will not include the request header data.
While this mechanism is not necessary for HTTP1, it is also updated to only send headers in the first application callback for consistency. However for HTTP/1 the first callback may also include body data in addition to the headers, since there isn't the same clear separation of headers and data into separate frames.
I've also made the
header_capture_ctxpart of thehttp_client_ctxregardless of whetherCONFIG_HTTP_SERVER_HEADER_CAPTUREis defined, since otherwise it would have been difficult to manage all of the#if defined(...)needed, and replaced withif(IS_ENABLED(...))where needed (has the bonus that the included code is compile checked regardless of which Kconfig options are defined). The buffers however are defined to a size of zero ifCONFIG_HTTP_SERVER_HEADER_CAPTUREis not enabled, so the impact on code/ram size is fairly minimal in case the option is disabled (<200 bytes each for the sample app on ARM Cortex M).While working on this fix I also spotted a second issue: since the
current_detailwas stored at a client level rather than a http2 stream level, concurrent HTTP2 POST requests would overwrite the resource detail and lead to data being either associated with the wrong resource detail, or not associated with any resource detail at all (giving a 404 response). This is fixed in the first commit.