search

Tectu / malloy

A cross-platform C++20 library providing embeddable server & client components for HTTP and WebSocket.
BSD 3-Clause "New" or "Revised" License
66 stars 8 forks source link

[Discussion] Why does `server::controller` use `shared_ptr` for the router, and why does `router` use `shared_ptr` for adding subrouters #101

Closed 0x00002a closed 2 years ago

0x00002a commented 2 years ago

While making changes for #68, something started troubling me: why does router use a shared pointer for adding subrouters? Is it valid to add the same subrouter to different parents with different controllers as the root? I think with server string overwriting certainly it shouldn't be, but if it is intended to be valid then its something we have to consider before implementing that (see: #68).

Personally I think it shouldn't be valid to do this, as I understand it routers essentially take ownership of subrouters when they are added to them, so it should either take a unique_ptr or by value (nicer imo, also avoids needing the dynamic allocation which is always nice).

Also I am a bit confused as to why controller returns its router as a shared pointer, is it valid for that router to be used after the controller is destroyed? Or for that router to be added as a child to another router? imo this should return a ref. While we're on the subject, why does controller use all shared pointers in its fields? Its copy and move constructors are both deleted, listener has to be a shared pointer but the other two, as far as I can tell, could just be values.

tl;dr I think we should remove all the shared pointers from the interface of router and server::controller because, as far as I can tell, they don't make sense with the ownership models in play

Thoughts?

Tectu commented 2 years ago

The router class is probably the "oldest" piece of code in this library. That's pretty much where it all started. After all, this was supposed to be an exercise in how to use boost.beast before actually doing so in three different projects. As it turned out, lots of boost.beast related code was shared between the projects which is where malloy became to be. The router class was serving more as a design entry point than anything else and as such it suffers from some quality issues.

To summarize: Yes, this is ugly - Yes, this needs to be cleaned up.

tl;dr I think we should remove all the shared pointers from the interface of router and server::controller because, as far as I can tell, they don't make sense with the ownership models in play

Thoughts?

I agree.

0x00002a commented 2 years ago

In that case, I can do this as part of the (hopefully final) fix for #68, since its going to be a breaking change for most router usages anyway (removes the ctor taking the server string)

Tectu commented 2 years ago

I'm quite okay with that. I made a precautionary release just a few hours ago. Looking at the state of things, I think we're on a good path. But I do foresee more breaking changes in the near future so lets get them over with ASAP :p

That being said - I was looking into creating a GitHub organization for this project - and subsequently I was wondering whether malloy might not be the best choice of names. I'm "concerned" that it might be "dismissed" or "fly too low under the radar" to become a reasonably accepted piece of software. Thoughts?

0x00002a commented 2 years ago

Not sure honestly :p. I don't know what about a name causes ppl to dismiss it or what exactly you mean by "fly too low under the radar", is there some context I'm missing about the name malloy?

Tectu commented 2 years ago

Nah, nothing you're missing. I just meant to inquire your opinion. I suck at naming stuff and I wanted to prevent a situation where people are looking at a software library named malloy and think that it's just whatever and move on. nginx does have a certain... expectation that it radiates. But I guess if apache is fine then malloy should be too :D

Tectu commented 2 years ago

Closing as this was addressed in #102.