Create socket with provided executor #909
Conversation
| fc::create_listener<Protocol>(thread_pool.get_executor(), _log, accept_timeout, address, "", | ||
| [this](const auto&) { return boost::asio::make_strand(thread_pool.get_executor()); }, | ||
| [this](Protocol::socket&& socket) { | ||
| boost::asio::post(app().get_io_service(), [this, socket{std::move(socket)}]() mutable { |
There was a problem hiding this comment.
Just for code conciseness, can we go back to placing the listener on main thread to avoid this post() entirely?
There was a problem hiding this comment.
Actually that doesn't work: https://github.com/AntelopeIO/spring/actions/runs/11238499086
We capture the this and we need the listener to shutdown when thread_pool.stop() is called in the plugin_shutdown. This can be cleaned up when we work: #842
There was a problem hiding this comment.
We capture the
thisand we need the listener to shutdown whenthread_pool.stop()is called in theplugin_shutdown.
I'm not immediately seeing why that's a problem. this is still alive after plugin_shutdown() completes. I think something else we're missing. Maybe the already-created strand (that is using ship's thread pool's executor) expects to touch that executor when it's destroyed when async_accept() is torn down when the main thread's io_context is destroyed which is after the plugin is destroyed and thus the thread pool's executor.
But yeah can wait to touch this further later.
|
I think it lgtm, but what are compelling reasons to make the change in 1.0? It certainly makes asio usage more idiomatic, which is good, but I don't know if we can point to something as being fixed or defective? |
I completely agree. I don't think this helps #816 (comment) . I asked the same question in the PR description. |
|
I think #816 (comment) was resolved with #845 |
Right. At least until we can get a better fix via: #842 |
heifner
changed the title
| strand( my_impl->thread_pool.get_executor() ), | ||
| socket( new tcp::socket( my_impl->thread_pool.get_executor() ) ), | ||
| strand( boost::asio::make_strand(my_impl->thread_pool.get_executor()) ), | ||
| socket( new tcp::socket( strand ) ), |
There was a problem hiding this comment.
Now that the socket is always being created with a strand (both here and via the listener), that should mean that all the bind_executors to the strand can be removed. Not sure you want to tackle that now or not (going in to 1.0 probably not, but going in to main maybe)
There was a problem hiding this comment.
I left it mainly because it was targeting 1.0. At this point, maybe it would be cleaner. In many places you would have boost::asio::post(socket.get_executor(), [](){}) instead which is not really any difference I don't think.
There was a problem hiding this comment.
You would have to move the timers to using the strand too (afaict not a problem) but after that it does seem like all 5 bind_executors could be removed. I am not seeing the need for boost::asio::post(socket.get_executor(), [](){}) in those same spots. Anyways, it's not something that needs to be done now just a clean up I noticed since we're constructing these more idiomaticly now.
There was a problem hiding this comment.
Oh, you were talking specifically about the bind_executor. I was thinking of the boost::asio::post(strand,.
| if (ec) { | ||
| fc_wlog(logger_, "Unable to retrieve local_endpoint of acceptor, error: ${e}", ("e", ec.message())); | ||
| } | ||
| acceptor_.async_accept(socket_executor_fn_(ep), |
There was a problem hiding this comment.
What is the purpose for the changes here and for passing the local endpoint as a parameter to the socket_executor_fn_? (this local endpoint parameter is not used by any of ship/http/p2p)
There was a problem hiding this comment.
I thought it might be useful. If someone was wanting to choose which executor according to which endpoint. I'm happy to remove it since we don't have a current use-case.
|
Note:start |
Modify
fc::create_listenerto take a lambda that provides the executor (often a strand) for the socket that is created. This makes sure the socket is associated with the desired executor.I don't see how this helps: #816 (comment) Maybe I'm missing something. If this doesn't help that issue, then seems like this should target
mainand not 1.0.3. <= Changed target tomain.Resolves #819