Inconsistent message lifetime

[redboltz]

mqtt::message and mqtt::v5::message's member variables are mixed reference type and value type.

For example,

• value
  • mqtt::basic_connect_message
  • https://github.com/redboltz/mqtt_cpp/blob/8f2fa503f11eb6b52b94b55727c7aecb2046b1fe/include/mqtt/message.hpp#L487
• reference
  • mqtt::basic_publish_message
  • https://github.com/redboltz/mqtt_cpp/blob/8f2fa503f11eb6b52b94b55727c7aecb2046b1fe/include/mqtt/message.hpp#L726
  • mqtt::basic_subscribe_message
  • https://github.com/redboltz/mqtt_cpp/blob/8f2fa503f11eb6b52b94b55727c7aecb2046b1fe/include/mqtt/message.hpp#L747

And mqtt::v5::basic_*_message properties are value.

It's very difficult to manage. Users need to care which member is reference or value.

Here is a table that describes reference/value.

message(basic_*_message)	type	note
connect	value	will topic and payload are value too
connack	value
publish	reference	v5 props are value
puback	value
pubrec	value
pubrel	value
pubcomp	value
subscribe	reference	v5 props are value
suback	value
unsubscribe	reference	v5 props are value
unsuback	value
auth	value
disconnect	value

Reference means boost::asio::const_buffer (as::const_buffer), and value means mqtt::buffer. Note that mqtt::buffer behaves as not only value but also reference. It depends on internal shared buffer.

I started thinking mqtt::basic_publish_message should have mqtt::buffer instead of as::buffer. mqtt::buffer could have reference type and value type (but behave as value type).

For this inconsistency, my college and I spent a lot of time for debugging my broker. It still continues.

For example, one of the basic_publish_message' constructor hasbuffer` parameter. https://github.com/redboltz/mqtt_cpp/blob/8f2fa503f11eb6b52b94b55727c7aecb2046b1fe/include/mqtt/message.hpp#L539 It is called for restoring message. The caller expects that the lifetime of the buffer is shared by basic_publish_message because the parameter type is buffer not stirng_view. But it doesn't. It is unexpected behavior. For v3.1.1, I can replace the parameter type to string_view. But for v5, https://github.com/redboltz/mqtt_cpp/blob/8f2fa503f11eb6b52b94b55727c7aecb2046b1fe/include/mqtt/v5_message.hpp#L614 if the message contains property, the lifetime of the buffer is shared for property. And by its side effect, topic and payload reference are still valid but user shouldn't depends on the behavior. If the property is not appeared, topic and payload reference is invalid. Caller need to manage their lifetime by caller side. It is not a implementation detail of mqtt_cpp. User call the constructor for restoring. It is very difficult to understand.

@jonesmz , could you tell me (again) what is the advantage that mqtt::message doesn't have buffer ? If it has buffer, the things become very simple. If the advantage is small performance benefit, keep things simpler is my priority.

[redboltz]

I don't say that basic_publish_message should always maintain the lifetime by itself. The lifetime can be managed by outside code such as life_keeper as follows:

https://github.com/redboltz/mqtt_cpp/blob/8f2fa503f11eb6b52b94b55727c7aecb2046b1fe/include/mqtt/endpoint.hpp#L2139 https://github.com/redboltz/mqtt_cpp/blob/8f2fa503f11eb6b52b94b55727c7aecb2046b1fe/include/mqtt/endpoint.hpp#L8652

It is good to me.

I just want to replace as::const_buffer with mqtt::buffer at https://github.com/redboltz/mqtt_cpp/blob/8f2fa503f11eb6b52b94b55727c7aecb2046b1fe/include/mqtt/v5_message.hpp#L847 and similar places.

For normal API call, such as async_publish, the buffer behaves as string_view. Because even if the parameter https://github.com/redboltz/mqtt_cpp/blob/8f2fa503f11eb6b52b94b55727c7aecb2046b1fe/include/mqtt/v5_message.hpp#L548 becomes mqtt::buffer, the caller can pass string_view as a buffer at https://github.com/redboltz/mqtt_cpp/blob/8f2fa503f11eb6b52b94b55727c7aecb2046b1fe/include/mqtt/endpoint.hpp#L8179.

But restoring situation, buffer has its lifetime. Restoring situation means, constructing the buffer using https://github.com/redboltz/mqtt_cpp/blob/8f2fa503f11eb6b52b94b55727c7aecb2046b1fe/include/mqtt/v5_message.hpp#L614 and buffer buf has lifetime.

After the modify I want to do, the existing code doesn't increase reference counter operation. What I want to do is not clear, I will send the PR to discuss.

[redboltz]

It is off topic that I noticed there are some other issues that I don't respond yet. I don't have much time due to the debug our broker. Sorry about that.

[jonesmz]

I am also quite busy.

I will try to respond properly within a few days.

[jonesmz]

Apologies for the delay in answering you.

The reason why I (even strongly, I suppose) recommend not explicitly requiring the message objects to have lifetime management inside of them is because it makes it difficult to use those message objects in a no-overhead way.

There's a couple of aspects to this:

1) Messages constructed entirely out of read only memory. If the message object itself had lifetime management, the message object is much larger than it would need to otherwise be, and this makes compile-time construction difficult (or impossible).

2) Messages constructed from buffer types that are not based on shared_ptr, but have some other lifetime system. the end-user may have the lifetime of buffers handled by things like, for example, co-routines (e.g. c++20 co-routines, boost::coroutune, etc), or the lifetime of all the buffers in a message may be identical, so if they supply us with an std::any{} to act as the life_keeper, but we require that they give us MQTT_NS::buffer, we're requiring either one nullptr per MQTT_NS::buffer, or we're adding one ref-count per MQTT_NS::buffer that the message object stores.

3) The existing mqtt_cpp user-facing api, where each parameter is passed in individually may not be the preferred level that the end-user wants to talk to mqtt_cpp. Right now that's the only API directly exposed to end-users, but we might eventually expose the do_sync_write and do_async_write functions directly as a "use at your own risk" capability. This is helpful for things like brokers, because many messages (e.g. publish) could potentially be forwarded directly to the subscribers without having to parse, decompose, and then re-compose a new message.

4) Complicates things like my proposed properties_cursor concept, and the iterator based interface that I've been working on (Unfortunately, very slowly because of other responsibilities, sorry for delay). When receiving an incoming message, I don't want mqtt_cpp to actually parse any of the properties, and I don't want it to allocate a vector and copy the properties into it. I just want it to receive the properties data into a buffer, and then have some struct properties_cursor that will parse the property data on demand (Possibly with caching of the parse results, of course). This properties_cursor will have a lifetime, because it will be reading from an MQTT_NS::buffer that manages the lifetime.

Then, on the send side, if the properties_cursor that I receive does not need to be modified (e.g. properties received on a publish message, being forwarded as-is) I want the same lifetime tracking to be used all the way until the message is sent on the wire.

E.g.

struct properties_cursor { MQTT_NS::buffer data; };
struct properties_iterator_adapter{ /* some struct that has a begin and end function and returns a property_variant for each item in the list */
receive_publish(topic, message, properties_cursor, lifetime)
{
    for(auto client : clients)
    {
        client.publish(topic, message, properties_iterator_adapter{properties_cursor}, lifetime);
    }
}

If I design the code for the above psuedo-code correctly, the entire operation involves only a single lifetime tracking object for the properties (e.g. MQTT_NS::buffer), and no construction of properties objects at any point in time, because the properties stay in the format that they were received in.

Compared to what we have now, this saves: 1) Allocation of vector<variant> when we receive the message. 2) Parsing of properties (maybe) 3) One MQTT_NS::buffer per property (but only the lifetime tracking, since we already support multiple properties pointing to the same MQTT_NS::buffer) 4) Allocation / copy of vector<variant> on publish() function. 5) serialization of properties (maybe) 6) several atomic-reference-count operations per property object.

---

Better design, in my opinion:

ConnectMessage
{
    boost::asio::const_buffer username;
    boost::asio::const_buffer password;
    MQTT_NS::properties_iterator_adapter properties;
    ....
    Other non-lifetime-owning member variables
}
do_async_write(ConnectMessage{}, lifetime{});

In this way, all message types become reference-only types. Lifetime is always tracked externally.

By always having the same behavior (e.g. always reference), it becomes simpler to work with the message types, and message types are always paired with a lifetime object, but that lifetime object can be simply std::any{} <- null lifetime, if the end-user wants to manage lifetime management themselves (e.g. with co-routines, or read-only memory, or something else).

[redboltz]

Thank you for the comment. I am still very busy so I didn't deeply consider yet.

But I understand that the following things:

1. Current status of the lifetime is transitional.
2. You want to go the way to reference types.
3. The key feature for reference type is property cursor.

Basically it is acceptable for me but I worry about two things.

1. If Property Length is greater than props_bulk_read_limit_, then property allocate memory individually. How property cursor works well with this functionality? Note that props_bulk_read_limit_ is 256 by default and user can set it.
   • I can't accept removing props_bulk_read_limit_ functionality. It is important for me.
2. How to pass properties to callable overlay? I guess that you can write an adaptor to match existing callback. I mean creating properties that contain memory managed buffer that is created from property cursor and lifetime.

Anyway, I will wait your PR for introducing property cursor.

[jonesmz]

Current status of the lifetime is transitional. You want to go the way to reference types. The key feature for reference type is property cursor.

Yes, this is the plan.

If Property Length is greater than props_bulk_readlimit, then property allocate memory individually. How property cursor works well with this functionality?

This is good question.

We will have to work together to design proper behavior.

How to pass properties to callable overlay?

I hope for API compatible version, not certain yet, still working.