Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Keep http_plugin_impl alive while connection objects are alive #9512

Merged
merged 3 commits into from
Sep 24, 2020

Conversation

vzqzhang
Copy link
Contributor

@vzqzhang vzqzhang commented Sep 21, 2020

Change Description

When the nodeos is killed, the thread_pool and associated io_context of the http_plugin_impl should keep alive for the lifetime of the connection object

Change Type

Select ONE

  • Documentation
  • Stability bug fix
  • Other
  • Other - special case

Consensus Changes

  • Consensus Changes

API Changes

  • API Changes

Documentation Additions

  • Documentation Additions

…their destructors need the io_context of the http_plugin_impl thread_pool io_context
@heifner
Copy link
Contributor

heifner commented Sep 21, 2020

Please use the standard template for the description.

@nksanthosh
Copy link
Contributor

@heifner Can you please review this PR? Thanks!

@heifner
Copy link
Contributor

heifner commented Sep 23, 2020

@nksanthosh this is all my code Qing just made the PR. Needs to be reviewed by someone else.

@@ -938,12 +951,15 @@ namespace eosio {
my->thread_pool->stop();
}

// release http_plugin_impl_ptr shared_ptrs captured in url handlers
my->url_handlers.clear();
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't quite enough. The thread pool's io_context may contain unexecuted handlers that hold shared_ptrs to the http_plugin_impl.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not following. The io_context destructor will destroy all the unexecuted handlers that hold shared_ptrs to http_plugin_impl.
https://www.boost.org/doc/libs/develop/doc/html/boost_asio/reference/io_context/_io_context.html

Do you just mean the comment is not clear? Or do you see an issue here?

Copy link
Contributor

Choose a reason for hiding this comment

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

You're only stopping the thread pool, not resetting it. This leaves a shared_ptr cycle.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. Thanks!

@vzqzhang vzqzhang merged commit 98c3f84 into develop Sep 24, 2020
@vzqzhang vzqzhang deleted the http-life-develop-rebase-develop branch September 24, 2020 16:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants