[Suggestion] Router shouldn't take the agent string in the constructor

[0x00002a]

The current behaviour of router requires one to pass in an agent string to the ctor every time a new one is constructed. This is gets a bit repetative and also lets the user have different agent strings per subrouter (which may or may not be a feature we want to keep).

I suggest removing that from the constructor and having it as a seperate method, which can be used by add_subrouter to set the agent string if there isn't one currently (if we want to keep the different string per subrouter an option). Alternatively we could have a default value for the agent string argument, since add_subrouter would be able to access the private members of the subrouter being added.

[Tectu]

Yeah, this has been annoying me the past few days as well :p I see no scenario where an agent string is needed on a per-router basis.

[0x00002a]

Well we do still need some way for the controller to get that information to the router. Could make it friend maybe? Or have it as a setter

[Tectu]

I've been thinking about this. Nothing elegant comes to mind.

I have two applications which heavily use sub-routers. I'm running into the problem that I most of the time manually keep track of the sub-routers target base as this information is also used to build stuff such as relative storage paths for file serving locations etc. I am thinking of adding this information as a field to router. We could then transform router::m_sub_routers to std::unordered_map<std::string_view, std::shared_ptr<router>>.

[Tectu]

I'm referring to #99 here:

This implements part of what I suggested in the issue while keeping the current behavior and features (namely different server string for subrouters). I'm not sure this is really desired functionality but since this is a fix I didn't want to remove functionality.

Personally, I see absolutely no reason to preserve this functionality. At least from my end this was never an intended design feature but rather a side effect of how this code evolved / came-to-be. As per your reasoning: There's no point in removing it if what we're after for is just a fix (given this particular circumstance).

For the future I do think that we should drop this non-sense and use something more decent such as an std::shared_ptr<std::string> to hold the server string. Calling router::add_subrouter() would simply provide the new sub-router with an instance of this smart pointer. This should clean up the design and also provide the means to change the server string during runtime (not that I can think of any situation where we'd want that but...) (we should probably use an atomic smart pointer then).

[0x00002a]

For the future I do think that we should drop this non-sense and use something more decent such as an std::shared_ptr to hold the server string [..]

Personally I don't like the idea of having a piece of shared mutable state threading through all the subrouters like that. We could instead opt for a string_view where the string is actually owned by the server controller. My concern is still though about the contructor which actually takes this, because it will be exposed to the user unless we do something clever and its an implementation detail if we arn't allowing it to actually do anything (which will effectively be what happens if we just overwrite it).

I don't particularly like the iteration required for propagation to subrouters but I also don't think a shared_ptr is the way to go, especially as like you said we'd have to make it threadsafe and that is almost certainly going to have an ongoing cost rather than just the upfront one during init (which doesn't matter much anyway imo, its done once and its not going to be in a hotpath ever). We could also just use a raw pointer which is const so it can't modify, set it to the string in the controller and only ever read from it to avoid the need for locking (that includes preventing contorller from updating it after startup).

None of the approaches though avoid the iteration though, as we still have to update the pointers or the values if we keep storing it as std::string

[Tectu]

Personally I don't like the idea of having a piece of shared mutable state threading through all the subrouters like that. We could instead opt for a string_view where the string is actually owned by the server controller.

Huge +1

We could also just use a raw pointer which is const so it can't modify, [...]

Yeah, let's please not do that :p

I fully get everything you're saying and I think it's safe to say that I pretty much agree. My notion of using an std::shared_ptr was meant as a possible solution to spark a discussion rather than making an actual decision. I started thinking about it only while typing that which is why my second thought was immediately: "damn... thread safety" but I decided to keep in in that post anyway for the sake of the discussion.

From what I can tell the most decent solution (at least for now) is to keep the std::string in the controller and passing std::string_view to the routers through whatever-appropriate-mechanism. To keep things simple, I'd really opt for keeping everything read-only: Let the user supply the string at controller creation time and that's it. If we (or somebody else) ever comes across the need to change this string during runtime we can extend the functionality. But until then I'd argue that keeping things as simple as possible is probably the way to go unless we know upfront that this is definitely gonna be needed functionality in the near future.