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

trigger input plugin shutdown using out of band call #3812

Closed
wants to merge 5 commits into from

Conversation

jsvd
Copy link
Member

@jsvd jsvd commented Aug 28, 2015

Currently, during shutdown, the pipeline loops through input workers
and calls Thread.raise on each input thread. Plugins rescue that
exception to exit the run loop. Afterwards, teardown is called for
any additional bookkeeping.

This proposal exposes a stop call on the input plugin class to alert
an input that it should shutdown. It is the plugin job to frequently
poll an internal stop? method to know if it's time to exit and
terminate if so.

The teardown method remains to do the additional bookkeeping, and it
will be called by the inputworker when run terminates.

resolves #3210 and replaces #3211

Currently, during shutdown, the pipeline loops through input workers
and calls Thread.raise on each input thread. Plugins rescue that
exception to exit the `run` loop. Afterwards, `teardown` is called for
any additional bookkeeping.

This proposal exposes a `stop` call on the input plugin class to alert
an input that it should shutdown. It is the plugin job to frequently
poll an internal `stop?` method to know if it's time to exit and
terminate if so.

The `teardown` method remains to do the additional bookkeeping, and it
will be called by the inputworker when `run` terminates.
jsvd added 2 commits August 28, 2015 13:52
In some scenarios it's necessary for the inputworker or the pipeline
to know that the plugin is in a shutdown mode
@@ -23,6 +23,7 @@ Gem::Specification.new do |gem|
gem.add_runtime_dependency "clamp", "~> 0.6.5" #(MIT license) for command line args/flags
gem.add_runtime_dependency "filesize", "0.0.4" #(MIT license) for :bytes config validator
gem.add_runtime_dependency "gems", "~> 0.8.3" #(MIT license)
gem.add_runtime_dependency "concurrent-ruby", "~> 0.9.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

ty for fixing a specific version. +1

Copy link
Member Author

Choose a reason for hiding this comment

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

well actually I'm saying anything 0.9.*

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I meant that :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I expect 1.0 to break a lot of stuff.

@jordansissel
Copy link
Contributor

Jenkins failure is due to build timeout. Can you investigate this cause?

@jordansissel
Copy link
Contributor

I tested this patch (and logstash-devutils#32) using logstash-plugins/logstash-input-pipe#10. The spec for having 'stop' terminate the plugin fails: logstash-plugins/logstash-input-pipe#10 (comment)

rescue => e
# if plugin is stopping, ignore uncatched exceptions and exit worker
if plugin.stop?
@logger.debug("Ignoring stopping plugin exception", :exception => e, "backtrace" => e.backtrace)
Copy link
Contributor

Choose a reason for hiding this comment

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

The wording here is confusing. Something like "Exception thrown while plugin was stopping, ignoring it." ?

@jsvd
Copy link
Member Author

jsvd commented Sep 2, 2015

@jordansissel it's normal for rake test:core to hang since some core tests use plugins that need refactoring, like logstash-input-generator

@purbon
Copy link
Contributor

purbon commented Sep 3, 2015

I'm wondering if we should notify somehow from the plugin side the fact that the plugin is ready to work. This could be helpful in some situations like:

This strategy could also work for slow stop process where might be interesting to be be sure the process is inside the stop method, or also in other situations like being sleeping. I think we might be thinking about this the same way as the thread status api, but in our case the plugin status api.

This might simplify the handling of some situations inside the plugins like the wakeup from sleeping.

What do you think?

@colinsurprenant
Copy link
Contributor

@purbon I am not sure how we would define the semantic of a kind of a ready? method. normally, a plugin is ready once it returns from the register method? if not, when is a plugin ready? since we're dealing with async stuff, when are we ready and for what?

Further:

  • I don' t think «a plugin got stalled during setup» is a problem that need fixing.
  • potential «slow stop process» is not related to this ready? idea which is related to the plugin startup.
  • not sure what you mean by a plugin «being sleeping»? if you mean waiting on a mutex or IO blocked, this is again not related to a ready? idea.

Please open a new issue to submit this idea if you think this is still valuable and we can discuss it there, let's keep the focus on this PR.

# as the first action
public
def stop
@logger.debug("stopping", :plugin => self)
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: maybe log "stop called" instead of "stopping" it seems to better represent the reality?

@colinsurprenant
Copy link
Contributor

If we are to break the API any reason to keep teardown around? IMO close is more appropriate and commonly used for this?

@colinsurprenant
Copy link
Contributor

moving discussion about a potential ready? method in #3885

@colinsurprenant
Copy link
Contributor

this PR is moving to #3895 and is now against the feature/oob_plugins_stop shared branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Design] plugins shutdown sequence refactor proposal
6 participants