Re: [pve-devel] [PATCH http-server 0/2] refactor HTTP request processing

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 17/02/2023 um 16:25 schrieb Max Carrara:
> This patch series refactors the request processing algorithm by
> separating its data and logic from one another in order to make future
> changes regarding requests more transparent and explicit.
> 
> Patch 1: Introduce the refactored algorithm
> Patch 2: Replace and remove the existing algorithm with the new one

See not much benefit in having this split into two patches in this way,
constructing (one or ideally more) patches such that the refactored (out)
code is directly put to use in(stead of) it's old place has normally more
benefits, even if just allows one to see what actually moved (e.g., using
the `--color-moved=zebra --color-moved-ws=ignore-all-space` git switches)

But no need to resend now, I can squash locally for doing a more in-depth
review first.

> 
> This refactor is conducted very carefully to ensure that none of the
> existing behaviours change in any way.

Careful as in? ^^ Doesn't seem to be a very conservative "just split up",
but more involved; and there isn't any testing added to ensure the changes
don't mess with semantics - which tbf might not be the easiest thing to do
in a sensible manner, that is actually covering a lot, here.

But especially header parsing with some edge cases could be quite nicely
done with the respective request methods mocked.

> One exception however would be
> the reading of the HTTP header itself: Instead of reading the
> header by recursively appending / prepending callbacks to the handle's
> and reading the header line-by-line, the entire header is read at
> once, reducing the number of callbacks needed to parse the header from
> the number of lines to one. This affects when the header line count
> and size limits are checked - instead of being checked *while* the
> header is being read, these limits are verified *after* the header
> was read from the read buffer. If this change in behaviour is
> undesirable, I'll gladly provide a v2.

that seems to make those limits mostly (i.e., besides maybe proxied
requests) irrelevant though? If one spent all of the resources to process
a huge header fully it doesn't makes sense to only then die if it's deemed
to big.

But again, I'd wait out for a more in-depth review before going on a v2.

> 
> Otherwise, the behaviour is identical; the logic is restructured,
> but entirely preserved.
> 
> 
> To give a more concrete example of what these changes would make more
> transparent, the enhancement requested in #4494[1] would become just
> another step (-> subroutine) in the refactored algorithm. That way it
> becomes much clearer where the TLS handshake is being verified
> and which responses (e.g. a `301`) are sent in case the handshake
> never happened or was aborted.

A patch, even if just POC, would be a more concrete example ;-) that could
then _actually_ show that the changes actually provide a nicer interface to
extend.

> 
> [0] https://bugzilla.proxmox.com/show_bug.cgi?id=4494
> 
> 
> pve-http-server:
> 
> Max Carrara (2):
>   http-server: anyevent: add new subroutine for processing requests
>   http-server: anyevent: replace request processing subroutine
> 
>  src/PVE/APIServer/AnyEvent.pm | 671 ++++++++++++++++++++++------------
>  1 file changed, 445 insertions(+), 226 deletions(-)
> 

puh, almost doubles SLOC...