Should `controller` use `optional` instead of `shared_ptr` for tls context

[0x00002a]

Currently the controller uses a std::shared_ptr for managing the TLS context. I'm guessing this is so it works with the forward definition. This works but shared_ptr has a not insignificant overhead compared to a pointer and of course requires dynamic allocation. unique_ptr would solve the overhead issue, but still has needless dynamic allocation. std::optional allows lazy construction of the context while ensuring it is stack allocated and serving as an almost drop-in replacement for shared_ptr.

Is there something I'm missing here?

[Tectu]

When first designing this, I was unsure of whether the TLS context will be needed somewhere outside the respective controller class. Therefore, a shared_ptr seemed more appropriate.

Other than that, as you mentioned, it ensures that we can use forward definitions. The overhead, from any perspective, should be negligible in my opinion anyway as objects of type controller tend to be created once during the application's lifetime.

As of today, I see no reason why not to migrate to std::optional.

[Tectu]

I looked into this & remembered why I settled for an std::shared_ptr: The TLS context gets passed to the listener. Personally, I would really much like it if in the future the server controller allows creating multiple listeners to listen on different interfaces & ports.

I haven't looked into whether the same asio::ssl::context can be used safely by multiple threads though. If we have to copy we can migrate to std::optional. Otherwise I think that the initial design relying on std::shared_ptr is the better way to go.

Thoughts?

[0x00002a]

Hmm, that does sound like shared_ptr would be good. But the client doesn't need shared_ptr, it could be unique_ptr at least. If the manager returns a std::unique_ptr instead then we can use it for the client (and shared_ptr can be constructed from unqiue_ptr). shared_ptr does have overhead esp with threads, but we may be verging on premature optimisation here :p.

Also, on whether asio::ssl::context is threadsafe: https://stackoverflow.com/a/33519766/12448530

[Tectu]

Yeah - I do think that exchanging a shared_ptr for a unique_ptr is premature optimization at this point, given the other open points of this library ^^

As you mentioned, currently both the client and server controller add their own m_tls_ctx fields. This could be moved to the agnostic controller base class - or do you see any reason not to do that?

[0x00002a]

Well from a design perspective is strikes me as inheritance to reuse, rather to be reused. It precludes changing the context type of client without also affecting server, which may be an issue since they have different lifetime considerations and such.

[Tectu]

I seem to be completely missing something here... Both client::controller and server::controller have the m_tls_ctx of type std::shared_ptr<boost::asio::ssl::context>. How do they differ?

I take it that you're talking about everything else around that field - such as the corresponding initialization & setup functions which differ between client & server, right? With that I completely agree.

[0x00002a]

I meant that now they do not differ, but moving it up prevents changes if they would need to differ in the future. Say because client::controller doesn't need it to be shared but server does. It doesn't enhance its ability to be reused, the base controller doesn't need it to provide its interface, it simply allows reuse by client and server.

[Tectu]

Gotcha - we agree on that then.

IMHO there's currently no need to change anything. I'm going to close this for now. If you disagree feel free to re-open this. Otherwise I'd keep optimizing on this level for when all the other parts of the library are polished enough.