Decompose endpoint.hpp into endpoint.hpp and endpoint_base.hpp

[jonesmz]

Right now we have a very large amount of functionality bundled into the same class, and I think we could enhance understandability by breaking this class into two parts.

EndpointBase <- This class would have all of the functionality that's completely generic to the type of message being sent/received. This includes the machinery for interacting with the socket directly, and the async read/write code.

Endpoint <- This class would build off of the low level machinery provided by EndpointBase.

I believe that decomposing the code like this would be 99% a cut-and-paste operation.

---

We could go even further, and have an EndpointClient and EndpointServer.

For example, there's not a lot of overlap between the functionality needed to support servers versus the functionality needed for clients.

Clients send connect, and receive connack, servers receive connect, and send connack. That type of thing.

---

What do you think?

If I opened a pull request to support something like the above, would you be interested in that?

[redboltz]

Right now we have a very large amount of functionality bundled into the same class, and I think we could enhance understandability by breaking this class into two parts.

EndpointBase <- This class would have all of the functionality that's completely generic to the type of message being sent/received. This includes the machinery for interacting with the socket directly, and the async read/write code.

Endpoint <- This class would build off of the low level machinery provided by EndpointBase.

I believe that decomposing the code like this would be 99% a cut-and-paste operation.

I think that it's worth trying. However, I'm implementing MQTT v5 functionality on the current endpoint.hpp. Please wait a moment. I will let you know.

[jonesmz]

Ok. Will wait.

[jonesmz]

So I think we need to discuss this concept more.

I spent a couple of hours today investigating what would have to happen, and i'm not sure the best way to implement this.

A few options:

1. Make the template class endpoint, accept two new template parameters. Sender, Receiver. This would allow the user of the class to switch between async or non-async implementations of the low level sending / receiving functionality. Right now I notice that receiving data always happens with async calls. I'm not sure one way or another if that's the best design, so I'm not real trying to address that.

Once the raw send/receive operations are handled by template calls, we can merge all of the various functions used to send data async/sync into a single set of implementations.

Problems with this approach: When doing blocking sends, typically lifetime doesn't need to be handled. By merging the calls into a single set of calls, we'll either have to always handle lifetime, or will need to have some other kind of magic to detect if lifetime needs to be handled.

2. Move functionality that isn't concerned with sending / receiving data into the base class. That's things like callback functions for data that have already been received (e.g., the handle_payload() function). The registration of the other various handlers, that kind of thing. Then, on top of that, we would have AsyncEndpoint.hpp, and SyncEndpoint.hpp. This allows for blocking / nonblocking functionality to live separately. E.g., instead of "async_publish()", it would just be "publish" and the specific implementation of it would be handled by whether or not the AsyncEndpoint was used, or the SyncEndpoint.

Personally, I'm not really sure there's a lot of value in supporting a synchronous API, but it's your project so we can keep supporting it.

I think that ultimately 3 layers might be best.

1. Generic logic, such as handling incoming data after it's been received. Think of this as a bunch of utility functions. Actually, it might be best to implement this as a bunch of utility functions.
2. Send/Receive logic, without regard to the MQTT protocol, just socket stuff
3. Protocol specific stuff that builds on the send/receive stuff in layer 2.

Thoughts?

[redboltz]

I'd like to allow to user both async/sync API call from the same endpoint object. Boost.Asio support it. I don't want to limit it on the upper layer, mqtt_cpp.

It seems that option 1 and 2 can't do that.

I think that we need to confirm what is the real problem with the big one file.

Some editor becomes slow with a big file. If it is the problem, simply separating file could be a solution.

I mean

• endpoint.hpp contains full of the class definition. The class definition contains member functions declaration but not contains definition.
• endpoint_impl_xxx.[hpp|ipp contains member function definitions. xxx can be various name. e.g. sync async publish subscribe acquired.
• endpoint.hpp includes endpoint_impl_xxx.[hpp|ipp]s from the bottom line.

[jonesmz]

I'd like to allow to user both async/sync API call from the same endpoint object. Boost.Asio support it. I don't want to limit it on the upper layer, mqtt_cpp.

But you made async_client.hpp and sync_client.hpp specifically because allowing sync and async on the same object doesn't work.

Further, right now, it's not possible to pass an mqtt::server instance to another type, like test_broker, and have the passed in instance decide if it's async or sync. In order to make test_broker.hpp use the server asynchronously, the code for test_broker.hpp has to be changed to explicitly call the async versions of functions.

I think that we need to confirm what is the real problem with the big one file.

• The file is difficult to navigate, making it hard to understand how things work. It's just a huge maze.
• Functionality is very closely intertwined, making it difficult to replace logic with alternative choices.
• A lot of repeated functionality (There are 16 versions of async_unsubscribe(), 6 of acquired_async_unsubscribe, and 4 acquired_async_unsubscribe_impl).
• Slower compile times. For example: I don't want to use the syncronous api at all. But my build still needs to pay the cost of parsing the code, even if the compiler doesn't generate machine code for it.
• The current code is very heavily coupled, but low cohesion. Endpoint.hpp does basically everything, but there are several parts of it that don't interact with other parts. There's several ways it can be broken up into smaller parts, it's just a question of which design to use.

endpoint.hpp contains full of the class definition. The class definition contains member functions declaration but not contains definition.

This would be an improvement, but it doesn't address all of my concerns.

[redboltz]

I'd like to allow to user both async/sync API call from the same endpoint object. Boost.Asio support it. I don't want to limit it on the upper layer, mqtt_cpp.

But you made async_client.hpp and sync_client.hpp specifically because allowing sync and async on the same object doesn't work.

It is just a wrapper. The purpose is avoid misuse. If I use client.hpp, I can use mixed operation.

Further, right now, it's not possible to pass an mqtt::server instance to another type, like test_broker, and have the passed in instance decide if it's async or sync. In order to make test_broker.hpp use the server asynchronously, the code for test_broker.hpp has to be changed to explicitly call the async versions of functions.

This is the functionality just I want to keep. Broker implementer can choose async_publish() or publish(). test_broker.hpp is just for test purpose. So I use sync one. The benefit of sync one is easy to implement. User doesn't need to care about resource management. It is good enough for test broker.

I think that switching sync/async from outside is not practical. Concept is slightly different.

When I use Boost.Asio, I can choose send() orasync_send()` via socket.

https://www.boost.org/doc/libs/1_70_0/doc/html/boost_asio/reference/basic_stream_socket/async_send.html https://www.boost.org/doc/libs/1_70_0/doc/html/boost_asio/reference/basic_stream_socket/send.html

I want to continue to provide the similar functionality on mqtt_cpp.

[redboltz]

I wrote the design decision introducing Sync and Async client. https://github.com/redboltz/mqtt_cpp/pull/208#issuecomment-480594751

Why don't you create asnyc_client and sync_client as the base classes of client?

• It requires big design change.
• I don't have good design concept.

I think that I can accept something good mixin approach.

Here is code image:

struct endpoint_base {};

struct async_endpoint {
    async_endpoint(endpoint_base& base):base(base) {}
    endpoint_base& base_;
};

struct sync_endpoint {
    sync_endpoint(endpoint_base& base):base(base) {}
    endpoint_base& base_;
};

struct endpoint : public sync_endpoint, public sync_endpoint {
    endpoint(endpoint_base& base):async_endpoint(base),  sync_endpoint(base){}
};

I think that ultimately 3 layers might be best.

Is this similar to your 3 layers approach?

[jonesmz]

A mixin approach would be similar to what I described, and I'd be happy to help implement that.

[redboltz]

Thank you.

• The file is difficult to navigate, making it hard to understand how things work. It's just a huge maze.
• Functionality is very closely intertwined, making it difficult to replace logic with alternative choices.
• A lot of repeated functionality (There are 16 versions of async_unsubscribe(), 6 of acquired_async_unsubscribe, and 4 acquired_async_unsubscribe_impl).
• Slower compile times. For example: I don't want to use the syncronous api at all. But my build still needs to pay the cost of parsing the code, even if the compiler doesn't generate machine code for it.
• The current code is very heavily coupled, but low cohesion. Endpoint.hpp does basically everything, but there are several parts of it that don't interact with other parts. There's several ways it can be broken up into smaller parts, it's just a question of which design to use.

I think that this approach can achieve the benefit you mentioned above. In addition, it can achieve providing both sync/async API on the one endpoint object.

I can accept async_endpoint has publish() function not async_publish() as you proposed. Mixed in endpoint can provide async_publish() as follows:

struct endpoint_base {};

struct async_endpoint {
    async_endpoint(endpoint_base& base):base(base) {}
    void publish(...); // async publish
    endpoint_base& base_;
};

struct sync_endpoint {
    sync_endpoint(endpoint_base& base):base(base) {}
    void publish(...); // sync publish
    endpoint_base& base_;
};

struct endpoint : public sync_endpoint, public sync_endpoint {
    endpoint(endpoint_base& base):async_endpoint(base),  sync_endpoint(base){}
    void publish(...) {
        sync_endpoint::publish(...);
    }
    void async_publish(...) {
        async_endpoint::publish(...);
    }
};

But I don't want to introduce virtual function based polymorphism.

I mean

struct endpoint_base {
    virtual ~endpoint_base() {}
};

struct async_endpoint : virtual endpoint_base {
    async_endpoint(endpoint_base& base):base(base) {}
    void publish(...); // async publish
    // endpoint_base& base_; // remove composition, use inheritance
};

struct sync_endpoint : virtual endpoint_base {
    sync_endpoint(endpoint_base& base):base(base) {}
    void publish(...); // sync publish
    // endpoint_base& base_; // remove composition, use inheritance
};

struct endpoint : public sync_endpoint, public sync_endpoint {
    endpoint(endpoint_base& base):async_endpoint(base),  sync_endpoint(base){}
    void publish(...) {
        sync_endpoint::publish(...);
    }
    void async_publish(...) {
        async_endpoint::publish(...);
    }
};

I can't accept the approach above.

---

I think that one of the big issue of mixed in approach is how to treat well std::enable_shared_from_this. And I don't have a good solution yet.

Anyway, let's start implementing.

I want to discuss based on small running code. https://wandbox.org/ will help us.