http_plugin boost::beast migration, keep-alive (EPE-9 / GH-3678) #10689
Conversation
move host_xxx_valid functions to common reorder constructor member init to avoid warning
code cleanup and disable some logging statments
respect keep-alive option on server and client request
plugins/http_plugin/include/eosio/http_plugin/beast_http_session.hpp
Outdated
Show resolved
Hide resolved
plugins/http_plugin/include/eosio/http_plugin/beast_http_session.hpp
Outdated
Show resolved
Hide resolved
| } | ||
| shared_ptr<beast_http_listener<plain_session, tcp, tcp_socket_t > > beast_server; | ||
| shared_ptr<beast_http_listener<ssl_session, tcp, tcp_socket_t > > beast_https_server; | ||
| shared_ptr<beast_http_listener<unix_socket_session, stream_protocol, stream_protocol::socket > > beast_unix_server; |
There was a problem hiding this comment.
No reason for beast_server, beast_https_server, or beast_unix_server to be shared_ptr, they can be unique_ptr.
There was a problem hiding this comment.
beast_http_listener calls async methods (async_accept) and so needs to be shared_from_this(), which implies they need to be constructed through a shared_ptr
There was a problem hiding this comment.
Calling async methods doesn't require shared_ptrs, it just requires that the object remains "alive" until completion. The shared_from_this() pattern is one way of accomplishing that by dragging the shared_ptr along as a captured variable in each callback. But that pattern tends to be best for ephemeral connection objects imo, because any alternative (that I'm aware of) tends to be clunky.
In this particular case, it does seem like from an architectural standpoint there wouldn't be a need for them to be shared_ptrs. As long as the thread pool is stopped & joined before the unique_ptr goes out of scope everything is safe because the listener is always alive while any asyncs are outstanding.
Also not sure why thread_pool has been promoted to a shared_ptr. shared_ptrs tend to cloud ownership semantics (the other comment about who is to call stop() on the thread_pool being a perfect example); I'm somewhat skeptical they are needed beyond usage of the shared_from_this() pattern on the connection objects.
There was a problem hiding this comment.
@spoonincode
On the surface, it seems that the listener objects should only live as long as the http_plugin object, which should "own" them. However, this is not necessary the case precisely because the listener/session objects are calling async methods, and the http_plugin object lives inside the main app thread, while the listener/session objects live in both the app thread and the thread_pool.
Because we never restart the http_plugin while the app is running, having the http_plugin object own the listener object outright, would be less of a problem. However, on shutdown, because they live in separate threads, the listener objects can very well have outstanding async calls after they would be destroyed by http_plugin object's destructor. In this case, it is likely a segfault or or an unhandled excpetion error will occur, causing in the terminate_scenarios_XXX and restart_scenarios_XXX to become flaky.
I had that exact problem with the plugin_state object which I tried to pass as a reference in some circumstances. I can inspect the PR history to show the flaky tests and the fix.
|
|
||
| void stop_listening() { | ||
| if(is_listening_) { | ||
| thread_pool_->stop(); |
There was a problem hiding this comment.
http_plugin::plugin_shutdown() is calling stop() on the thread_pool already. Doesn't seem appropriate to do it here, as http_plugin/impl is seemingly the "owner" of the thread pool since it created it
| private: | ||
| bool is_listening_ = false; | ||
|
|
||
| std::shared_ptr<eosio::chain::named_thread_pool> thread_pool_; |
There was a problem hiding this comment.
Seems like these could just be references instead of shared_ptrs?
Remove shared_ptr to logger
Misc cleanup EPE-9
Change Description
Overhauls http_plugin, replacing websocketpp with boost::beast. Feature parity with previous implementation, with exception of new additions keep-alive/multiple requests per session and additional server-side error logging. Additional test in plugin_http_api_test for keep-alive.
#10583
Change Type
Select ONE:
Testing Changes
Select ANY that apply:
Consensus Changes
API Changes
Clients which set "Connection: keep-alive" in the HTTP header will bel able to send multiple requests per session, if "http-keep-alive" is enabled in nodeos. This is enabled by default. There is no changes for any clients which set "Connection: close".
Documentation Additions