Renaming controller_run_result

[Tectu]

I'm really not happy with the naming scheme used by the newly introduced controller_run_result. It doesn't really reflect the design of the system appropriately in my opinion.

I'd propose renaming controller_run_result to just controller and renaming the current controller class to controller_factory.

@0x00002a Thoughts?

[0x00002a]

For future readers, see my comment in #106 for my thoughts on how we could do this with a session object.

I think there is an interesting issue around how we deal with client controllers. It makes sense for the server one to be a factory if it sets up the routes and they're static one its starts, but the client controller needs to be able to dynamically make new requests. What we could do is have the controller factory and the session both expose the same api for making requests, but then we have to write the same api twice and maintain both versions, unless we want to inherit from some base which I am conflicted about since imo it is an abuse of inheritance but it is also used in stuff like beast (message inherits from header for example) and would let us provide that factory version on the client side while preserving the dynamic stuff

[0x00002a]

I've been thinking a bit more on this, and I came up with another option: make the client api start the runner on construction. For example, make the start function take a config struct directly and have it return the session (or just have the session constructor take it, but the wrapper lets us have symmetry between the server and client). This is somewhat intuitive with the rest of the usage, rather than being something which can be both setup and dynamically used it becomes something which is always dynamically used. We would of course need some way to init tls, for example an overload which takes a tls config struct or something.

To avoid duplication across the client and server, we could use template-based dependency injection to abstract the provider of the io context and thread spawning, this would have the added bonus of letting users specify their own way to run the io context and have direct access to it (although how this interacts with the work guard is something that still needs thinking about imo)

Whether we go this second route or not, I think we need to very carefully consider if we want to allow both the setup and run behavior and the run then setup usage.

[Tectu]

I like your approach.
To be perfectly transparent here: I found ZERO amount of time the last three days to look into this properly/decently. It's a constant itch at the back of my head tho... It literally keeps me awake at night.

@0x00002a are you able to spin up a draft for this?

[0x00002a]

@0x00002a are you able to spin up a draft for this?

I'll take a crack at it today, probably won't get very far but should at least get more of an idea of the issues

[0x00002a]

I've got through a bit of this proof of concept, but I am feeling a little afraid I'm making such a massive change, my implementation has removed the server controller and made start take a router as the first arg instead (with an optional tls context arg). I've renamed client::controller to client::session and made it start up on construction. I just ain't sure these changes are really justified or even a good idea.

Maybe it would be better to just rename them, I'm worried I'm making substantual changes which arn't needed :p. Any thoughts?

[Tectu]

Maybe it would be better to just rename them

That's definitely what I had on mind from the beginning :p
Personally, I think what we have in terms of structure is quite reasonable. What bugs me is mainly the naming of the classes/structs/types that we're using in the current design.
I think that the current structure/design is pretty reasonable and - while maybe not the most minimalistic/elegant solution - I'd argue that it's also sufficiently future proof.

[0x00002a]

@Tectu How about keeping controller named the same, but changing the type alias they expose to be named session. On the client side this makes sense imo, as the controller can keep being used after started but I agree on the server it is less intuitive. session_factory or controller_factory are both a bit unhelpful though imo, I quite like something along the lines of routing_context but its a little verbose :p.

[Tectu]

@0x00002a that sounds reasonable. I wouldn't even mind routing_context. I'm also okay with calling the type returned by the server controller::start() just session so it's consistent with the client. After all, it's like a serving session.

[Tectu]

@0x00002a I've been thinking about this (again): Why not just renaming routing_context to context? Then we'd have malloy::server::context which seems pretty intuitively. Thoughts?