Extract HttpUserAgent class from HttpSM

[JosiahWI]

This extracts a new class with 3 fields (entry, raw_buffer_reader, and txn). It doesn't have much responsibility yet. But it does provide a starting point for moving some behavior out of the state machine.

Edit: We extracted a lot more fields, and moved HttpVCTable to its own file.

[maskit]

I'm fine with making changes step by step, but I'd like to see the big picture before making changes.

HttpSMHandler, HttpVC_t and HttpVCTableEntry probably need a better place.

[JosiahWI]

My objective is to move some of the logic for response headers into this new class. For example, HttpSM::setup_push_read_response_header() can be moved entirely or in part into this new class.

[maskit]

Hmm, the goal is still unclear to me. If it's going to be like we can get anything about a user agent from HttpUserAgent, and anything about a server from HttpServer that kinda make sense. But we already have client_info and server_info in HttpTransact::State. What would be the difference? And why don't you move other variables that are related to client (e.g. client_request_hdr_bytes, client_tcp_reused, client_protocol`, etc) into the class?

[JosiahWI]

I wanted to split up the class a bit based on the single responsibility principle to make it more manageable. I did not have a long-term vision of how the refactoring would continue, but I'd like to try moving the other client fields over (in this PR) like you suggested, and splitting out HttpServer in a separate one.

I'm pretty new to the codebase and it's not very clear to me yet which fields belong together. I thought client and user agent were somehow different concepts in the codebase, because they are named differently.

[JosiahWI]

The goal is to encapsulate these fields such that the state machine doesn't need to access this data at all. If it has less data it has to manage directly it will be a lot easier to follow the logic. I want to move operations on client and server data out of the state machine, so that the state machine focuses on changing between states correctly and delegates the details of changing the state to those new classes.

[maskit]

Thanks for the clarification.

I wanted to split up the class a bit based on the single responsibility principle to make it more manageable.

Sounds reasonable. You've already answered my question on the other comment but I was not sure what the responsibility of the new class where you have only one for user agent side.

I'm pretty new to the codebase and it's not very clear to me yet which fields belong together. I thought client and user agent were somehow different concepts in the codebase, because they are named differently.

I'd say nobody fully understand the concepts 🙂 The two terms might have been used for a client on HTTP layer and a client on TCP layer, but this is just my guess.

The goal is to encapsulate these fields such that the state machine doesn't need to access this data at all. If it has less data it has to manage directly it will be a lot easier to follow the logic.

Sounds great. Let's make it happen.

[JosiahWI]

Rebased on master due to merge conflict.

[JosiahWI]

Rebased on master.

[bryancall]

[approve ci autest]

[JosiahWI]

The autest failure on this one indicates a real problem. I've been working on it but debugging the state machine without unit tests is a relatively large time sink.

[JosiahWI]

I found the bug. I missed a single !, and I think I spent as much time finding that single missing ! as I spent making this entire PR. I'm going to add a unit test that covers that case, but I'm encountering a lot of segfaults in the test setup.

[JosiahWI]

Rebased on master. Some unfinished changes also got pushed by mistake.

[JosiahWI]

Rebased on master to resolve merge conflict.

[JosiahWI]

I'm wondering whether it's worth it to build the new test in the automake build. In the CMake build it was convenient to build a single test executable, but the automake build has things split into multiple test executables, and building the one new test would require a large amount of boilerplate (linking 10+ libs or whatnot), and I'm hoping the automake build will eventually be removed anyway so we don't have to maintain two build generators.

[maskit]

I wondered if we should keep this because now we can do sm->get_user_agent()->get_txn(). Maybe we should keep it, so we can get a user-agent transaction and a server transaction in a consistent way.

[maskit]

Looks good. Assuming functions are just moved and there's no logic change.

[JosiahWI]

There isn't supposed to be any logic change. The AuTests passing and the relative simplicity of the change (most of the code was directly copy/pasted) gives me a little confidence that the behavior is preserved, and I am planning to add some more unit tests in a followup PR.