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

Async plugin startup #38

Merged
merged 6 commits into from
Nov 16, 2018
Merged

Async plugin startup #38

merged 6 commits into from
Nov 16, 2018

Conversation

heifner
Copy link

@heifner heifner commented Nov 2, 2018

  • add signal handling thread so startup is interruptable
  • move setup of signal handling from exec() to startup() so that plugin startups are interruptable.

- add signal handling thread so startup is interuptable
application.cpp Outdated
@@ -63,8 +65,19 @@ bfs::path application::get_logging_conf() const {

void application::startup() {
try {
for (auto plugin : initialized_plugins)
plugin->startup();
auto ioc = io_serv;

Choose a reason for hiding this comment

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

what's this for? looks unused

Copy link
Author

Choose a reason for hiding this comment

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

It is a copy of the shared_ptr<io_service> captured in post()

Choose a reason for hiding this comment

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

ah yeah missed that it was a shared_ptr

- add signal handling thread so startup is interuptable
application.cpp Outdated
plugin->startup();
auto ioc = io_serv;
for (auto plugin : initialized_plugins) {
// order of execution for single-threaded io_service is order of post

Choose a reason for hiding this comment

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

What are the odds that one day the io_service will not be single-threaded? Could we write this job in a way that it tail-posts the next job instead of posting all jobs and relying on something that may drift in the future? That way the safety is guaranteed by the implementation of this method and not the environment.

Copy link
Author

Choose a reason for hiding this comment

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

There is actually no reason not to just post the initialized_plugins startup loop itself. Updated PR.

Copy link
Author

Choose a reason for hiding this comment

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

As per our discussion. Moved setup of sig handler thread to startup() and move plugin startup calls back to caller thread (main thread).

@heifner heifner merged commit 1d6e6e4 into master Nov 16, 2018
@heifner heifner deleted the async-startup branch November 16, 2018 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants