RXI-893 Change owning model to guarantee destroying sequence #104
Conversation
There was a problem hiding this comment.
I'm not following how this is safe - it looks like destroyed objects could be accessed in asio during system shutdown. But I didn't dig in too deeply, I thought I'd ask for an explanation for why this is safe first.
The commit message is too terse. The commit message should explain why it is safe to make this change.
| make_Federator( | ||
| App& app, | ||
| boost::asio::io_service& ios, | ||
| config::Config const& config, | ||
| ripple::Logs& l) | ||
| { | ||
| auto r = std::make_shared<Federator>( | ||
| auto r = std::make_unique<Federator>( | ||
| Federator::PrivateTag{}, app, config, l.journal("Federator")); | ||
| r->init(ios, config, l); |
There was a problem hiding this comment.
The motivation for the "make_Federator" and the separation of the constructor into a constructor and an init was to satisfy the constraints around "shared_from_this". If the shared pointers are going away, then this can be simplified.
There was a problem hiding this comment.
It is required quite a rewrites to make federator as object, and not a pointer, cause of the deferred initialization. Just don't want to make unnecessary changes.
| @@ -69,7 +68,7 @@ class WebsocketClient : public std::enable_shared_from_this<WebsocketClient> | |||
| std::atomic_bool peerClosed_{true}; | |||
|
|
|||
| std::function<void(Json::Value const&)> onMessageCallback_; | |||
| std::atomic<std::uint32_t> nextId_{0}; | |||
| std::atomic_uint32_t nextId_{0}; | |||
There was a problem hiding this comment.
I prefer the template versions of these functions, but also OK as-is.
There was a problem hiding this comment.
There are both in the code, so I refactor them when I see one.
| timer_.expires_after(CONNECT_TIMEOUT); | ||
| timer_.async_wait([wptr](boost::system::error_code const& ec) { |
There was a problem hiding this comment.
I see several places where a shared or a weak pointer was replaced with a naked this in a callback. The motivation for the shared/weak pointers was to make sure we don't access a destroyed object in asio. For example, during shutdown, the app is destroyed before asio. Aren't we in danger of accessing destroyed objects?
There was a problem hiding this comment.
Now Chain listener wait for websocketclient destructor. And websocketclient destructor will cancel all the io operations, and wait for it. So when destructor finish, no pending operations left. Then federator waits for both chainlisteners, and app waits for federator. These changes are to be ensure that when we destroy the local object, it is actually destroyed and there are no side effects caused by callbacks from the io operations.
Websocket destructor will cancel io operations and wait for it, so after destructor finished no pending tasks will remain. The pointers that are passed to callbacks will always point to valid objects. ChainListener now waits for websocketclient destructor, Federator waits for both chainlisteners and App waits for Federator.
No description provided.