Overhaul of `router` and `server::controller`

[0x00002a]

This has quite a wide list of changes, I made them 1 pr because they are all major breaking changes so I thought it would be a good idea to have them all at once.

Internal changes

• server::controller now uses a unique pointer for its m_tls_ctx, with m_router as a value field
• manager::make_context now returns a unique_ptr
• router now uses unique_ptr for the storage of endpoints

Major changes

• server::controller::start is now &&, meaning the user must do std::move(<instance>).start(), this is to hint that the controller should not be reused after a call to start, which was true before but now even more so as its internal state is moved out of it
• server::controller::router now returns a [const] router&
• router has been made moveable
• router::add_subrouter now takes a unique_ptr<router> or router&& (which just wraps the unique_ptr overload)
• The constructor taking a server string for router has been made private and server::controller is now a friend

Bug fixes

• router's now propagate logger instances in the same way as the server string, except it doesn't overwrite existing ones
• It should no longer be possible to add a controller's router to another router as a subrouter, due to the deleted copy constructor

Note that I have not touched listener or its pipeline, it still uses shared_ptr for most stuff (inc the tls context, unique_ptr can convert to shared_ptr when moved), I did not change this as I am unsure about the lifetimes involved, can a listener survive in a valid state without a controller? Can a connection survive longer than the listener that created it? I'm not really sure but either way it should be changeable later without breaking anything as its an internal api (or at least I think so?)

[Tectu]

I'll try to review this tonight - but looks like good work! As such, comments below are based on me not having reviewed this yet.

server::controller::start is now &&, meaning the user must do std::move(<instance>).start(), this is to hint that the controller should not be reused after a call to start, which was true before but now even more so as its internal state is moved out of it

I'm not sure whether I'm slightly concerned about this. One of the goals of malloy would be to provide a very easy to use high-level API which allows less seasoned C++ developers to use this library to include HTTP & WS server (and client) capabilities into their projects without too much hassle (i.e. a similar experience as one would get with similar Java & Python libraries. This might somewhat stretch that goal. Maybe we can provide some wrapper template or similar for this? In any case - technical correctness is more important in my opinion.

[...], can a listener survive in a valid state without a controller? Can a connection survive longer than the listener that created it? I'm not really sure but either way it should be changeable later without breaking anything as its an internal api (or at least I think so?)

These are questions I asked myself too when I first started writing this. I wasn't sure either which is why I decided to go with std::shared_ptr as that is usually the easiest to migrate to something else (at least IMHO). From a purely technical point of view I do think that a listener should be able to do its job without a controller. The same would go for a connection staying alive without a listener: In the event of a server shutdown one might want to terminate the listener immediately while still processing open connections. Whether that makes sense is a different question - this is something we should definitely look into. I see few reasons that would justify keeping this as a feature.

[0x00002a]

I'm not sure whether I'm slightly concerned about this [..]

Yeah I am not 100% on it either. Really I would like it to be a compile error to use it after start but since c++ lacks destructive moves (unlike e.g. rust) I don't think this can be done. The alternative then is to not require the move but that is also non-ideal as static anaylisers can't catch that and its not clear from the code its invalid. Note that before, calling start() after calling stop would result in weirdness since the work guard would be gone, and calling start again before calling stop would result in (I'm guessing) an exception as it tried to bind to an already in use address.

Honestly I feel like the controller interface as a whole could do with some improvement. Ideally it shouldn't be possible to access methods that arn't valid for the objects current state. One option could be to move init to the constructor, then have a free (friend) function that does the start and returns another object with the stop method. Its not perfect because it would still be possible to try and start twice but I think c++ programmers are much better at spotting use after move when its as a param rather than just calling a method. This could also be an oppotunity to introduce the CompletionToken stuff to stop which is currently non-trivial due to the inheritance going on with controller.

That's just my rough thoughts off the top of my head, but I think theres potential at least. The main issue I can see is that we would need to mirror them with client::controller which would break even more stuff, and it may honestly be overengineering a bit. It would however give us the ability to have state stuff the currently is inherited to be seperate from the controllers completely (e.g. in the stop object) which would remove the need for the inheritance completely (I think?)

[0x00002a]

So I tried implementing this thing with a separate class returned from start. Overall I think it works pretty well, all the state that used to be in malloy::controller can be moved to it and it prevents calling stop on a controller which isn't even inited yet. It also separates the actual running server from the information to create a running server at the type level which I think is nice.

Only major thing I've run into is that it means the result of start must be stored as the server will stop running once the dtor is called. Also I don't know if this fits with what you want for the api, since it changes controller to be more of a setup class rather than the overall thing doing both the setup and managing the lifetime of the run.

Finally one more thing I found is that listener takes its io context by &, which means that actually it isn't valid for it to live on past the controller that created it, whether we want to then remove its ownership of stuff like the router and just make it reference fields in the controller is I think a question of whether we want to it be usable standalone or not

[Tectu]

So I tried implementing this thing with a separate class returned from start. Overall I think it works pretty well, all the state that used to be in malloy::controller can be moved to it and it prevents calling stop on a controller which isn't even inited yet. It also separates the actual running server from the information to create a running server at the type level which I think is nice.

That actually sounds like a nice solution. I like it!

Only major thing I've run into is that it means the result of start must be stored as the server will stop running once the dtor is called. Also I don't know if this fits with what you want for the api, since it changes controller to be more of a setup class rather than the overall thing doing both the setup and managing the lifetime of the run.

I advocate for technical correctness. Currently we have the freedom to publish breaking changes whenever we want (which will be a different story once we hit the 1.0 release). I'd like to have a look at your implementation but I'd argue that we can create/rename the controller class to controller_factory and the type returned by start() might be the new controller - although that doesn't make complete sense - just thinking out loud here. What is important in my opinion is that we're able to re-use an existing i/o context.

Finally one more thing I found is that listener takes its io context by &, which means that actually it isn't valid for it to live on past the controller that created it, whether we want to then remove its ownership of stuff like the router and just make it reference fields in the controller is I think a question of whether we want to it be usable standalone or not

That is.... very true. Without knowing the actual implementation I'd say that ideally the I/O context would be maintained externally (if none was provided the controller (or controller_factory) will create one. The lifetime of the I/O context would therefore be "linked" to the object returned by the instance returned by start() - does that make sense?

[0x00002a]

Without knowing the actual implementation I'd say that ideally the I/O context would be maintained externally (if none was provided the controller (or controller_factory) will create one. The lifetime of the I/O context would therefore be "linked" to the object returned by the instance returned by start() - does that make sense?

This is actually how it works with my current implementation :p, it stores the io_context as a unique pointer in the returned object and passes it by ref to the listener.

I'd like to have a look at your implementation but I'd argue that we can create/rename the controller class to controller_factory and the type returned by start() might be the new controller - although that doesn't make complete sense - just thinking out loud here.

Do you want me to move them to a new branch, or push them to this pr now?

[Tectu]

This is actually how it works with my current implementation :p, it stores the io_context as a unique pointer in the returned object and passes it by ref to the listener.

Once again we find ourselves on the same page - excellent :p One thing tho: If it stores a unique pointer we won't have the ability to re-use an existing I/O context which the consuming application might already have around. This is something I was planning to have. Could we make that a shared pointer instead?

Do you want me to move them to a new branch, or push them to this pr now?

I don't really mind. If it's good enough to be merged into main put it in here :p

[0x00002a]

One thing tho: If it stores a unique pointer we won't have the ability to re-use an existing I/O context which the consuming application might already have around. This is something I was planning to have. Could we make that a shared pointer instead?

The problem with reusing an io context from the user is that we currently start up the io context with however many threads are specified in the config and with a work guard for the server to stop it shutting down when it isn't serving connections. I'm not sure how we could reasonably take a user io context and use it given these constraints, but we could provide a way to access it (in a non-owning way imo, since the threads it runs on are owned by the object)

[0x00002a]

Ok I've pushed the changes implementing this new api for the controllers. Its quite rough and I haven't fixed the examples or removed the controller base class because I wanted to get your thoughts on the changes first.

Changes

• there is now a start(...) friend function for both controller types, which should only be called once but we might want to add an explicit flag and check for this
• start(...) returns a manager object which controls the lifetime of the io context for whatever tasks each controller are setup for, in the case of client::controller it takes ownership of the one it currently has (which is why m_ioc is a raw pointer in client::controller). The implementation of stop has been moved to the dtor of this object, meaning it can only happen once
• init has been removed for both controllers, and moved to the constructor
• the controllers no longer inherit from base, instead the shared code has been moved to the returned object from start which is templated to allow custom state storage for each controller
• the object returned from start has a run method which is the implementation moved from client::controller, one accidental bonus here is that we got this for free for server::controller

Note that client::controller may be used after start, which is why it has to do the weird pointer stuff with m_ioc, calling start multiple times is still not valid though

[Tectu]

init() has been removed for both controllers, and moved to the constructor

I'm not a huge fan of that. I know that malloy is relying on exception handling in many places but I really like the idea of being able to catch initialization errors without exceptions. I'm not sure whether we can get malloy into an exception less library but I really like the thought of being able to do that one day. So wherever easily possible, I try to keep an init() function instead of throwing in the constructor. Is there a technical limitation behind this change (I haven't reviewed the new changes yet)?

the controllers no longer inherit from base, instead the shared code has been moved to the returned object from start which is templated to allow custom state storage for each controller

I like that! The common base class was indeed a limiting situation which needed to be rectified at one point.

While there are indeed a few details that need to be further investigated and potentially addressed in the (near) future I think this is worth committing too as your changes greatly improve the overall design which is definitely going towards the direction I want it to go.

[0x00002a]

I'm not a huge fan of that. I know that malloy is relying on exception handling in many places but I really like the idea of being able to catch initialization errors without exceptions. I'm not sure whether we can get malloy into an exception less library but I really like the thought of being able to do that one day. So wherever easily possible, I try to keep an init() function instead of throwing in the constructor.

Theres not a technical limitation but it lets the object be used in an invalid state, which is what I was trying to reduce as much as possible with these changes. This is the tradeoff for init methods. Personally I don't see the issue with exceptions but I respect if you would prefer to keep them out of the malloy, having said that I think being able to throw in constructors is really valuable since it allows this kind of code (correct by construction, literally). If exceptions are really a no-go one option could be to have a static factory method that returned an optional to signal errors or make use of another library to provide some kind of result type (which would let us provide a message)

[Tectu]

I get your point. I don't have personal reasons for this. I do come from the world of embedded systems engineering (think proper embedded systems, not the newfangled RaspberryPi type deals with 4GB of memory). These platforms are usually so resource limited that exception handling needs to be straight up disabled. This might be very far fetched, but I really love the thought of one day being able to use malloy on an embedded system :D That being said - I leave this one up to you. This is a minor implementation detail which we can change at any time in the future. There are other places in malloy which would require similar changes to make it even just remotely fit for this kind of use so we'd have to do this anyway.

So, this is all looking pretty great! Do you feel like finishing that up and then we merge it into main? The next thing I'm possibly gonna tackle is #77. I'm just not yet happy with the new design I have in mind.

[0x00002a]

These platforms are usually so resource limited that exception handling needs to be straight up disabled. This might be very far fetched, but I really love the thought of one day being able to use malloy on an embedded system :D

Ah that makes sense. All my c++ experience is with "normal" user applications so I didn't consider this

So, this is all looking pretty great! Do you feel like finishing that up and then we merge it into main?

Yep, I'll hopefully have this wrapped up in a few hours if nothing else major crops up (taps wood)

[0x00002a]

One last thing, I'm re-adding the validation that existed in init before, I'm wondering if I should make it throw always or make it terminate in debug mode and throw in release. I'm leaning towards just throwing since the second one is something I think we should consider for all the validation in malloy and done later, but I just wanted to check

[Tectu]

Ah that makes sense. All my c++ experience is with "normal" user applications so I didn't consider this

There are other things you often can't do on those resource restricted embedded systems. One other notable thing is RTTI. Compiling C++ code on something like a Cortex-M based system almost always come with these compiler/linker flags:

target_compile_options(
    ${TARGET}
    PRIVATE
        $<$<COMPILE_LANGUAGE:CXX>:-fno-exceptions>
        $<$<COMPILE_LANGUAGE:CXX>:-fno-rtti>
)

There are plenty of other C++ "things" you often can't use on those systems.

One last thing, I'm re-adding the validation that existed in init before, I'm wondering if I should make it throw always or make it terminate in debug mode and throw in release. I'm leaning towards just throwing since the second one is something I think we should consider for all the validation in malloy and done later, but I just wanted to check

Personally I'd just go with throwing always. When debugging, I set my debugger to break on any exception being thrown so you catch them easily. Furthermore, there are basically just two cases (assuming exceptions are being used):

• your surrounding code catches the exception and handles it appropriately so you'll know anyway.
• your surrounding code doesn't catch the exception and you'll end up with a termination anyway.

[0x00002a]

Don't merge yet, still missing some validation stuff on the controller ctors, also I noticed some of the examples are just broken currently I'm going to open a separate issue for that though as they are broken even before these changes

[Tectu]

[...], also I noticed some of the examples are just broken currently I'm going to open a separate issue for that though as they are broken even before these changes

Looking forward reading about that. I know that the application framework (APPFW) example is not complete yet (work in progress) but it should certainly build and work - and that should be the case for other examples too so I'm keen on your info here.

[0x00002a]

Still don't merge, still testing and I may have done a dumb with some of the example "fixes"

[Tectu]

Still don't merge, still testing and I may have done a dumb with some of the example "fixes"

ack - just tell me here when you think this is PR is ready for reviewing / merging :)

[0x00002a]

OK well on further testing it seems I haven't broken the examples, although the reason is not obvious. See I changed all the while(true) loops at the end of the server blocks to use run() instead, forgetting that run() also clears the work guard which is there (I thought) precisely because the io context should just return straight away since there are no actions, except there actually are actions because the listener is constantly queuing more of them. So actually it seems we don't need the work guard at all, I'm tempted to remove it but this pr has already had so much changes creep I'm not sure.

Thoughts?

[Tectu]

Thoughts?

Interesting... I haven't though of this when we talked about the work guard before. That being said, I do seem to remember a particular case which required the workguard to be present. But that might just be a side-effect from a past relic.

If this PR is safe & sound from your perspective I'd review & merge this and prefer to have the workguard situation in a separate issue/discussion/PR.

[0x00002a]

If this PR is safe & sound from your perspective I'd review & merge this and prefer to have the workguard situation in a separate issue/discussion/PR.

Sounds good to me, this should be safe and sound, all the tests pass and the basic http and websocket examples seem to work ok.

[Tectu]

I'll be sure to review & merge this within the next hours!

[Tectu]

Waiting for CI to complete but looks good to me!