Cleanup (http-server): refactor and tidy HTTP server implementation

[clagix]

• Refactored the HTTP server to store the entire request struct (method, target, headers, body) on connection_state->request_
• Remove unused method process_full_request (which seems to be replaced by the almost identically method process_full_request_with_conn)
• Removed now unneeded parameters and lambda captures.

[wilkolbrzym-coder]

Nice cleanup on the request handling! I took a look at the implementation and noticed a critical issue with the request_ state persistence.
Since conn->request_ now lives for the lifetime of the connection, we have a header leak on Keep-Alive connections. The headers map isn't cleared between requests, so Request B will inherit headers from Request A. We need to call conn->request_.headers.clear() (and probably clear the body too) at the start of process_request_data to ensure a clean state.
Also, handle_websocket_upgrade still uses the oldsend_error_response path, which doesn't handle the connection state or socket closing, potentially leaving the connection in limbo on failure.
We should probably address these before merging. Thoughts?

[clagix]

Thanks — you're absolutely right. I've added clear() calls to reset the request structure before processing the next request when using "Connection: keep-alive".
I also refactored the handle_websocket_upgrade and handle_streaming_request methods. I'm not a WebSocket expert, so I'd appreciate someone reviewing the updated error handling, in particular the use of error_response_with_close().

[wilkolbrzym-coder]

It looks solid! Resetting the request state was crucial for keeping things alive, and the new error handling for WS/streaming is much more reliable.

One minor note: the old send_response methods (for raw sockets) seem to be unused now and can be removed.

[stephenberry]

Thanks @clagix for this contribution, and @wilkolbrzym-coder for looking over this PR. I'll try to look over this soon to get it merged.

[wilkolbrzym-coder]

Thanks @clagix for this contribution, and @wilkolbrzym-coder for looking over this PR. I'll try to look over this soon to get it merged.

You're welcome! Glad I could help.

[clagix]

Thanks, @stephenberry, for considering the PR, and thanks, @wilkolbrzym-coder, for the review and helpful suggestions!

[clagix]

I’ve also removed the unused send_response() method.

I’d prefer to move all *_with_conn() methods into the connection_state struct. From an OOP perspective, it seems cleaner to place methods on the type they primarily operate on (aligning with the single responsibility principle) rather than encoding that relationship in the method name.

What do you think about this approach, @stephenberry and @wilkolbrzym-coder ?

[wilkolbrzym-coder]

I’ve also removed the unused send_response() method.

I’d prefer to move all *_with_conn() methods into the connection_state struct. From an OOP perspective, it seems cleaner to place methods on the type they primarily operate on (aligning with the single responsibility principle) rather than encoding that relationship in the method name.

What do you think about this approach, @stephenberry and @wilkolbrzym-coder ?

While the OOP approach is cleaner, process_full_request needs access to the router owned by the server. Moving it to connection_state introduces coupling complications (server dependencies).
Given that this PR fixes a logic bug/security issue, I'd prefer to merge it as-is (with the current structure) to get the fix in. We can refactor connection_state in a dedicated, separate PR.

[clagix]

@wilkolbrzym-coder you’re right. We should wrap up this PR and prepare it for merge. The primary goal of this PR is to improve code readability, with some minor improvements to security and performance.

We can then prepare a follow-up PR to move the _with_conn methods and evaluate any new dependencies or couplings introduced by those changes.

[stephenberry]

I reviewed the changes and I think this cleanup is in a good place. More cleanup and optimization can happen in future requests. So, I'll merge it in. Thanks for the contributions, it's always nice to simplify code!

[clagix]

@stephenberry

We could probably pass conn->request_ directly to start() here. I left the req variable in place to be safe, since I’m not deeply familiar with the websocket internals.

// Create request object for WebSocket handler (move since conn is consumed)
         request req{std::move(conn->request_)};