Deadlock during DhtRunner destroying

[novemus]

The problem occurs when the join method is invoked in the working thread of the runner. In my case it happens from the destructor. The 'shutdown` method is called before destruction.

Top of stack

Bottom of stack

The stack is too long, so I cut it.
One more thing. This happens if you call the run method with 0 port argument. I have not encountered the problem when I specify port explicitly.

PS. I was wrong. This also happens with an explicitly specified port.

[aberaud]

DHT callbacks are called by the DhtRunner thread.
For this reason, It is not supported to call join() or to destroy a DhtRunner instance directly from a DHT callback, as this would lead to a deadlock.

From the provided backtrace, it looks like the last shared_ptr<DhtRunner> to your DhtRunner instance was possibly captured in a lambda for a listen() callback at the time cancelListen() was executed. This would cause the captured shared_ptr<DhtRunner> and the DhtRunner itself to be destroyed, and cause a deadlock.
If that is indeed what's happening, the underlying problem is essentially a shared_ptr self-reference loop causing the DhtRunner being destroyed from a DHT callback.

A possible pattern to manage this scenario might be to capture only a weak_ptr<DhtRunner> (or weak_ptr<OtherAppComponent>) in the callbacks in order to avoid possible memory leaks or deadlocks, and to hold a shared_ptr<DhtRunner>and call join() in a higher-level component to manage the lifetime.

[novemus]

A possible pattern to manage this scenario might be to capture only a weak_ptr<DhtRunner> (or weak_ptr<OtherAppComponent>) in the callbacks in order to avoid possible memory leaks or deadlocks, and to hold a shared_ptr<DhtRunner> in a higher-level component to manage the lifetime.

Yes, that's right. In my case, the last instance of the shared_ptr<DhtRunner> is destroyed in the listen callback function. Now I will take this behavior of the DhtRunner into account in my code. However, this behavior is not obvious and the need to control the thread of objects destruction is not exactly what you would expect, especially in the epoch of smart pointers. In my opinion, this is a miss of design.

And in general, capturing mutexes on callbacks is also not very good, since you cannot control the time of this call and what happens in it. In multithreaded applications, this is a direct route to deadlocks. I've encountered similar problems myself.

[aberaud]

A possible pattern to manage this scenario might be to capture only a weak_ptr<DhtRunner> (or weak_ptr<OtherAppComponent>) in the callbacks in order to avoid possible memory leaks or deadlocks, and to hold a shared_ptr<DhtRunner> in a higher-level component to manage the lifetime.

Yes, that's right. In my case, the last instance of the shared_ptr<DhtRunner> is destroyed in the listen callback function. Now I will take this behavior of the DhtRunner into account in my code. However, this behavior is not obvious and the need to control the thread of objects destruction is not exactly what you would expect, especially in the epoch of smart pointers. In my opinion, this is a miss of design.

And in general, capturing mutexes on callbacks is also not very good, since you cannot control the time of this call and what happens in it. In multithreaded applications, this is a direct route to deadlocks. I've encountered similar problems myself.

Yes, but note that this is not specific to OpenDHT, it's a common design challenge with C++:

• std::shared_ptr doesn't handle reference loops, it's up to the developer to avoid shared_ptr loops. weak_ptr provides a tool for this, but it has to be managed explicitly.
• An object (using threads or not) typically can't properly destroy itself in C++, even when it's technically possible it's dangerous and has to be managed very purposefully (this can't ever be accessed after destruction). It's normally up to parent objects to manage the lifetime of child objects.
• Callbacks called from child objects are a common cause of deadlocks in many languages, because it favors lock patterns like childMutex -> parentMutex in the callback while another thread locks parentMutex -> childMutex. DhtRunner is implemented in a thread-safe way which avoids this problem, allowing to perform DHT operations directly from DHT callbacks without deadlocks, except destroying the DhtRunner itself from a DHT callback (because the runner and callback would destroy themselves).

Note that calling shutdown() is possible from a DHT callback and is enough to stop the node. join() or destroying the DhtRunner instance needs to be done from another thread, just like, for instance, you can't destroy an asio::io_context or other event loops from within that event loop callback.
When using smart pointers to manage DhtRunner, calling join() explicitly from a parent thread allows to ensure no other DHT callback will run after that, and that all callbacks objects have been destroyed. This provides a tool to avoid possible leaks, even when capturing shared_ptr in callbacks, and to avoid deadlocks by making sure the dht won't destroy itself.

Since the DHT doesn't have knowledge of other threads and event loops used by the user, it can't automatically reschedule callbacks on a different thread. Callbacks would always be called from within the DHT, preventing destruction directly from the callback.
A possible pattern to manage this may be to manually reschedule DHT callback on another thread or even loop.

I'm open to suggestions to make this clearer or easier to manage in OpenDHT.