Fixes #400: Change the accept handler for mqtt::server to accept a shared pointer instead of a reference

[jonesmz]

No description provided.

[jonesmz]

@redboltz I'm stuck on this.

Any chance you can take a look at some of the remaining unit test issues?

[redboltz]

I think that this kind of shared_ptr capture make a resource leak.

[redboltz]

Suggested change

	[&, spep]
	[&, &ep = *spep]

And

                    close_proc(connections, subs, ep.shared_from_this());

However, this is difficult to understand.

I think that if there is no other easy to understand solution, revert this change is better. That means give up to pass shared_ptr to accept_handler.

[redboltz]

I don't want to enforce these kind of reset process to users.

[jonesmz]

This is the consequence of using std::function as the behavior customization mechanism, instead of virtual functions. For almost all of the things the callback functions can be used for, there needs to be some way for the callback function to access the instance that is calling the callback function.

This is how the problem can be solved (no particular order):

1. Smallest change -- provide an mqtt_cpp::endpoint::clear_all_handlers() function, which sets all of the handlers to nullptr. This ensures that when clear_all_handlers() is called, any remaining references to the std::shared_ptr<mqtt_cpp::endpoint> are released. This is part of basic shared_ptr management, to provide a mechanism to clear the references to a shared pointer held by a class so that the end user can break reference cycles in their code.
2. Use std::weak_ptr in all of the callback capture lists. -- This is unattractive because it will require that each call site use the following code, as boilerplate. This also increases the complexity, because if the weak pointer points to a null sharedptr, what should be done? Hard to decide.

std::shared_ptr<endpoint> spep = weakptr.lock();
if( ! spep)
{
    // The current callback code goes here.
}

3. Use references -- This is unattractive because of the strong possibility of dangling references. There no way to be certain that the reference is valid when the callback is called. At least with std::weak_ptr, the end user can check that the std::weak_ptr is valid before calling other functions. In the current code for test_broker, I see functions called with invalid references regularly, which causes lots of problems that I'm try to fix. Using std::weak_ptr is strongly preferred over using references, in my opinion.
4. Modify each handler so that they provide "this" as a parameter to the callback. This is my second-most favorite. This would make sure the callbacks always know which instance of mqtt_cpp::Endpoint is calling the callbacks. No need to check for std::weak_ptr reference count. No lifetime management / cycle breaking, no extra variables in capture lists.
5. Use virtual functions -- This, in my opinion, is the most attractive, but also the largest amount of code change, so I understand that it seems like a very large change. The reason why this is attractive is that it reduces the amount of code in mqtt_cpp::endpoint significantly (no need to check if a handler is valid, just call the virtual function directly), automatically ensures that the callback knows which "this" is being used, and also doesn't prevent the use of std::function for callbacks.
6. 4+5 together.

Example:

template<typename ...>
struct Endpoint : public std::enable_shared_from_this<DerivedName>
{
    virtual void on_connect(...) = 0;
    virtual void on_connack(...) = 0;
    virtual void on_publish(...) = 0;
    virtual void on_puback(...) = 0;
    ...

    void process_connect_impl(...) { ...; this->on_connect(...); ...; };
    void process_connack_impl(...) { ...; this->on_connack(...); ...; };
    void process_publish_impl(...) { ...; this->on_publish(...); ...; };
    void process_puback_impl(...) { ...; this->on_puback(...); ...; };
};

template<typename ...>
struct ClientBase : public Endpoint<...>
{
    // All of the current contents of mqtt_cpp::Client go here
};

template<typename ...>
struct Client : public Endpoint<...>
{
    // set_connect_handler(...); not provided, only server.
    void set_connack_handler(...); // Same as set_connack_handler from current mqtt_cpp::Endpoint
    void set_publish_handler(...); // Same as set_publish_handler from current mqtt_cpp::Endpoint
    void set_puback_handler(...); // Same as set_puback_handler from current mqtt_cpp::Endpoint


    // on_connect(); only for server
    void on_connack(...) override { if(h_connack_handler_) { h_connack_handler_(...): } }
    void on_publish(...) override { if(h_publish_handler_) { h_publish_handler_(...): } }
    void on_puback(...) override { if(h_puback_handler_) { h_puback_handler_(...): } }
    /// And so on for all of the other handlers.
};



template<typename ...>
struct ServerBase : public Endpoint<...>
{
    // All of the current contents of mqtt_cpp::Server go here
};

template<typename ...>
struct Server : public Endpoint<...>
{
    void set_connect_handler(...);  // Same as set_connect_handler from current mqtt_cpp::Endpoint
    void set_connack_handler(...); // Same as set_connack_handler from current mqtt_cpp::Endpoint
    void set_publish_handler(...); // Same as set_publish_handler from current mqtt_cpp::Endpoint
    void set_puback_handler(...); // Same as set_puback_handler from current mqtt_cpp::Endpoint


    void on_connect(...) override { if(h_connect_handler_) { h_connect_handler_(...): } }
    void on_connack(...) override { if(h_connack_handler_) { h_connack_handler_(...): } }
    void on_publish(...) override { if(h_publish_handler_) { h_publish_handler_(...): } }
    void on_puback(...) override { if(h_puback_handler_) { h_puback_handler_(...): } }
    /// And so on for all of the other handlers.
};

This gives users of the library the choice of

1. Use existing callback mechanism to provide the customization points for their application
2. Use inheritance to provide the customization points for their application.

For my application, I never change the callback during execution, so I have no reason to want callbacks. I would rather have virtual functions. It would make my code simpler, faster to compile, and probably less bugs (my bugs, not mqtt_cpp's bugs)

Pros/Cons of callbacks with the current codebase

• 1 function-pointer per std::function.
• 1 allocation per std::function that has a variable capture list (most, but not necessarily all of them).
• End Users must provide for reference count management themselves, using either std::shared_ptr, std::weak_ptr, or references.
• Ability to change callbacks during session lifetime

Pros/Cons of virtual functions, with the code I propose above:

• 1 vtable pointer in the base class -- unless the compiler devirtualizes the vtable pointer
• One pointer dereference for the vtable lookup per virtual function call -- unless the compiler devirtualizes the call, which is very possible with modern compilers and header-only libraries.
• No need for callbacks to hold a reference/pointer to the mqtt_cpp::endpoint instance, provided automatically as the "this" parameter.

The virtual functions have much better performance (potentially 0 overhead, thanks to compiler devirtualization) than std::function callbacks. Std::function<> callbacks have much better flexibility.

I strongly believe that using virtual functions in mqtt_cpp::Endpoint, and then providing std::function<> callbacks in the Client/Server layers is superior to what's being done now, as it provides exactly the same flexibility, but doesn't force end users to use std::functions if they don't want to.

Another way to do this that provides even more flexibility to the end user is with the curiously recurring template pattern:

template<typename ...>
struct Endpoint : public std::enable_shared_from_this<DerivedName>
{
    virtual void on_connect(...) = 0;
    virtual void on_connack(...) = 0;
    virtual void on_publish(...) = 0;
    virtual void on_puback(...) = 0;
    ...

    void process_connect_impl(...) { ...; this->on_connect(...); ...; };
    void process_connack_impl(...) { ...; this->on_connack(...); ...; };
    void process_publish_impl(...) { ...; this->on_publish(...); ...; };
    void process_puback_impl(...) { ...; this->on_puback(...); ...; };
};

template<typename ...>
struct Client : public Endpoint<...>
{
    // All of the current contents of mqtt_cpp::Client go here
};

template<typename ...>
struct Server : public Endpoint<...>
{
    // All of the current contents of mqtt_cpp::Server go here
};

template<typename IMPL_T>
struct CallbackOverlay final : public IMPL_T
{
    void set_connect_handler(...);  // Same as set_connect_handler from current mqtt_cpp::Endpoint
    void set_connack_handler(...); // Same as set_connack_handler from current mqtt_cpp::Endpoint
    void set_publish_handler(...); // Same as set_publish_handler from current mqtt_cpp::Endpoint
    void set_puback_handler(...); // Same as set_puback_handler from current mqtt_cpp::Endpoint

    // Here the IMPL_T may be end-user provided. E.g.
    // struct impl : public Client { /* overrides for the virtual functions here */ };
    // So we ensure to call the virtual function before the std::function
    // so that the end-users code still gets called.
    void on_connect(...) override final
    {
        IMPL_T::on_connect(...); // Call base class implementation
        if(h_connect_handler_)
        {
            h_connect_handler_(...):
        }
    }
    void on_connack(...) override final
    {
        IMPL_T::on_connack(...); // Call base class implementation
        if(h_connack_handler_)
        {
            h_connack_handler_(...):
        }
    }
    void on_publish(...) override final
    {
        IMPL_T::on_publish(...); // Call base class implementation
        if(h_publish_handler_)
        {
            h_publish_handler_(...):
        }
    }
    void on_puback(...) override final
    {
        IMPL_T::on_puback(...); // Call base class implementation
        if(h_puback_handler_)
        {
            h_puback_handler_(...):
        }
    }
    /// And so on for all of the other handlers.
};

And then you would use it like this:

namespace mqtt_cpp
{
    decltype(auto) make_overlay_client(...)
    {
        return std::make_shared<CallbackOverlay<AsyncClient>>(...);
    }
}
void main(...)
{
    auto pClient = mqtt_cpp::make_overlay_client(...);
}

With the above pattern, the end-user only pays for the callback handlers if they use them, and exactly the same API can be the "default", so that the people using the code now won't need to change anything when they upgrade.

---

@ropieur

I'd like to invite you to comment on this subject, as you've commented on other aspects of the handler style in other issues recently.

[redboltz]

You implement clear_all_handlers(). I want to avoid manual clear handler call. The original code doesn't need to call clear_all_handlers() thanks to RAII.

My priority is avoid calling clear_all_handlers(). It is higher priority than set_accept_handler() 's parameter to be shared_ptr<endpoint_t>.
Just we can do the idiom pass start_session() to shared_ptr<endpoint_t>.

[redboltz]

Or capture as std::weak_ptr at set_xxx_handler().
It can avoid loop reference.
My point is avoiding loop reference. I don't accept permitting loop reference and manually clear approach.

[jonesmz]

It's important that a function like clear_all_handlers() is provided by mqtt_cpp::endpoint so that users of the library can implement the lifetime management that they want to implement. If we don't provide this function, (even if you don't want to use it), then end users have to call each set_xyz_handler() function individually.

Personally, I strongly disagree with the start_session() function deliberately holding a reference to the mqtt_cpp::endpoint. That makes it very easy for end users to accidentally leak the mqtt_cpp object.

As demonstrated by my changes to test_broker that track the mqtt_cpp::endpoint object (the "accepted but not connected" collection), there were several places where we weren't tracking the lifetime of the accepted endpoints.

The original code doesn't need to call clear_all_handlers() thanks to RAII.

I'm not sure that's true. Adding the calls to clear the handlers fixed several crashes in my version of the test_broker.hpp. That doesn't mean that the handlers were the actual problem, but clearing them did make the crashes go away shrug.

---

Or capture as std::weak_ptr at set_xxx_handler().

Better would be either 4., 5., or 6. of my list above.

If you want to use weak ptrs, you're welcome to make that change, but I'm not sure I'm going to have time to implement that.

[redboltz]

Thank you for checking in depth. It seems to be very good workaround.

[jonesmz]

One last godbolt link on this PR, to keep it with the others.

https://godbolt.org/z/TnJ4BH

with [[gnu::always_inline]], it's not even necessary to use the final or override keywords.

[redboltz]

I'm reading your code #444. It seems that the existing code doesn't need to change thanks to callable_overlay. In addition, it is achieved with zero overhead on major compilers including gcc. It's nice. I need more time to understand the code. And I'm going to test the code on my browser. Please wait a couple of days.

[jonesmz]

Ok. Thank you for investigating with me.

[redboltz]

s/my browser/my broker/

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@4b832b4). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master     #430   +/-   ##
=========================================
  Coverage          ?   85.01%           
=========================================
  Files             ?       40           
  Lines             ?     6379           
  Branches          ?        0           
=========================================
  Hits              ?     5423           
  Misses            ?      956           
  Partials          ?        0

[redboltz]

Before discussing the PR, #435 should be fixed.
It is directly related to the session management from the server side.
Please wait a moment.

[redboltz]

@jonesmz , thank you for waiting. Now I merged all underlying fixes for this PR.
Could you rebase the PR from the master?

And let's consider what is the best way to keep session's lifetime.

start_session(spep) is a key feature. And spep shouldn't captures by handlers of ep such as set_*_handler() because it causes loop reference.

[redboltz]

If you want to use weak ptrs, you're welcome to make that change, but I'm not sure I'm going to have time to implement that.

Thanks.

I create the new PR #443 using shared_ptr and weak_ptr approach.
It doesn't contain clear_all_handlers() because I'm not convinced.

However, using wp.lock() instead of ep reference is easy to detect unexpected life time over problem.

[jonesmz]

Superceeded by #443

[ropieur]

Not sure I hold all cards in hand to debate, but a reason also for passing the handler as an argument to async calls is to give the compiler the opportunity to "inject" a lambda directly in the calling site and would give it the opportunity for more aggressive optimization. But this requires to avoid type erasure (e.g not to push the lambda inside a std::function).
I agree with @jonesmz on the fact that a std::function has a heavy cost. OTOH, I am not in favour of forcing a derivation from a "virtual interface" (java like approach).
Maybe, you could require a "concept" (with a class requiring a set of methods responding to the same requirements as virtual interface without the virtual aspect) that could be injected at once (so no need for std::function and no hard relation ...).

[jonesmz]

Lets move the discussion here: #444

[ropieur]

Do you suggest me to repost my previous comment in #444 ?

[jonesmz]

No need. I quoted you in my comment on #444