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

Handle signals during plugin_initialize #39

Merged
merged 2 commits into from
Jan 28, 2025
Merged

Conversation

heifner
Copy link
Member

@heifner heifner commented Jan 28, 2025

Spring creates the controller in chain_plugin::plugin_initialize. If ctrl-c is pressed during chain_plugin::plugin_initialize chainbase can be left with a dirty-db flag.

  • Move initialization of signal handing to appbase::initialize so that it is in affect during plugin_initialize.
  • Do not cancel and restart the signal_set to remove SIGHUP from the signal set. This was technically not thread-safe before. The boost docs indicate shared objects are not thread-safe. Since we discussed before that there was no real reason to treat SIGHUP differently during startup, I just removed the cancel/restart. This makes it thread-safe and simplifies the implementation.

@heifner heifner added the OCI Work exclusive to OCI team label Jan 28, 2025
@greg7mdp
Copy link
Contributor

I'm wondering, in chain_plugin_impl::plugin_startup(), we can do long processing like loading a snapshot I think. Can we still ^C out of it?

@heifner
Copy link
Member Author

heifner commented Jan 28, 2025

I'm wondering, in chain_plugin_impl::plugin_startup(), we can do long processing like loading a snapshot I think. Can we still ^C out of it?

Yes, the signal handling is now in affect from appbase::initialize on, including chain_plugin_impl::plugin_startup().

@@ -386,6 +373,9 @@ bool application_base::initialize_impl(int argc, char** argv, vector<abstract_pl
std::string plugin_name;
auto error_header = [&]() { return std::string("appbase: exception thrown during plugin \"") + plugin_name + "\" initialization.\n"; };

// setup handling of SIGINT/SIGTERM/SIGPIPE/SIGHUP during initialize
Copy link
Member

Choose a reason for hiding this comment

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

I suppose this comment should have SIGHUP removed because setup_signal_handling_on_ioc() doesn't handle SIGHUP now

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I actually updated the comment that was removed.

@spoonincode
Copy link
Member

spoonincode commented Jan 28, 2025

I don't think you can ^C out of loading a snapshot because I don't believe snapshot loading code polls is_quitting(). Instead the ^C will just be queued up to be handled after it's done

EDIT: well technically not queued up since it was already delivered to the async_wait thread

@heifner
Copy link
Member Author

heifner commented Jan 28, 2025

I don't think you can ^C out of loading a snapshot because I don't believe snapshot loading code polls is_quitting(). Instead the ^C will just be queued up to be handled after it's done

Related: AntelopeIO/spring#798 (comment)

@heifner heifner force-pushed the init-signal-handling branch from 5b61671 to bbd74cc Compare January 28, 2025 02:05
@heifner heifner merged commit 74ac065 into main Jan 28, 2025
4 checks passed
@heifner heifner deleted the init-signal-handling branch January 28, 2025 12:05
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.

3 participants