Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IPv6 support #1203

Merged
merged 10 commits into from
May 22, 2023
Merged

IPv6 support #1203

merged 10 commits into from
May 22, 2023

Conversation

huangminghuang
Copy link
Contributor

@huangminghuang huangminghuang commented May 18, 2023

This PR refactors net_plugin, http_plugin, and state_history_plugin to support IPv6.

Resolves #622

@huangminghuang huangminghuang marked this pull request as ready for review May 18, 2023 20:04
@heifner heifner added the OCI Work exclusive to OCI team label May 18, 2023
@heifner heifner added this to the Leap v5.0.0-rc1 milestone May 18, 2023
plugins/http_plugin/http_plugin.cpp Outdated Show resolved Hide resolved
plugins/http_plugin/include/eosio/http_plugin/common.hpp Outdated Show resolved Hide resolved
plugins/net_plugin/net_plugin.cpp Outdated Show resolved Hide resolved
libraries/libfc/include/fc/network/listener.hpp Outdated Show resolved Hide resolved
libraries/libfc/include/fc/network/listener.hpp Outdated Show resolved Hide resolved
libraries/libfc/include/fc/network/listener.hpp Outdated Show resolved Hide resolved
libraries/libfc/include/fc/network/listener.hpp Outdated Show resolved Hide resolved
libraries/libfc/include/fc/network/listener.hpp Outdated Show resolved Hide resolved
plugins/net_plugin/net_plugin.cpp Outdated Show resolved Hide resolved
@heifner heifner requested a review from spoonincode May 18, 2023 20:36
@@ -98,7 +96,7 @@ class beast_http_session : public detail::abstract_conn,
// HTTP response object
std::optional<http::response<http::string_body>> res_;

std::shared_ptr<http_plugin_state> plugin_state_;
http_plugin_state* plugin_state_;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way to refactor this to not store a naked pointer?

} else if (ec == boost::system::errc::too_many_files_open) {
// retry accept() after timeout to avoid cpu loop on accept
auto timeout_ms = static_cast<T*>(this)->accept_timeout_ms;
fc_elog(state_->get_logger(), "accept: too many files open - waiting ${timeout}ms", ("timeout", timeout_ms));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe consider elaborating on this message a little more as I'm not sure everyone understands files == connections in this context. "open file limit reached: not accepting new connections for next Xms" or something like that

I'd like to figure out a way to suggest the action for user to take but I can't think of a concise way to put it.. "open file limit reached: not accepting new connections for next Xms, consider increasing 'ulimit -n'" .. but someone using docker, etc, would adjust this knob differently so don't really like that

inline auto make_http_response_handler(std::shared_ptr<http_plugin_state> plugin_state, detail::abstract_conn_ptr session_ptr, http_content_type content_type) {
return [plugin_state{std::move(plugin_state)},
inline auto make_http_response_handler(http_plugin_state* plugin_state, detail::abstract_conn_ptr session_ptr, http_content_type content_type) {
return [plugin_state = plugin_state,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it dangerous to use a regular pointer to http_plugin_state in a response_handler, instead of a shared_ptr that extends its lifetime if necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The handlers can't be executed after the plugin shutdown, and the state will only be deleted by the destructor of the plugin, so it's safe to do so.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change to http_plugin_state& then.

plugins/net_plugin/net_plugin.cpp Outdated Show resolved Hide resolved
plugins/net_plugin/net_plugin.cpp Show resolved Hide resolved
@huangminghuang huangminghuang merged commit 605f600 into main May 22, 2023
@huangminghuang huangminghuang deleted the huangminghuang/ipv6 branch May 22, 2023 19:29
@spoonincode spoonincode mentioned this pull request Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCI Work exclusive to OCI team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IPv6 support
4 participants