pipeline shutdown OOB plugins stop

[colinsurprenant]

This is a PR reboot of #3812 from a shared branch to facilitate collaboration.

Original description by @jsvd :

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

[colinsurprenant]

pushing commits related to started?, ready? and @run_mutex cleanups as discussed in #3812 (comment) and #3886

[purbon]

@colinsurprenant @jsvd Added jordansissel/ruby-stud#23 to track work on making Stud more interrupt friendly.

[talevy]

why not get rid of close/teardown in favor of just using stop for both stop signalling and cleanup?

Sometimes they are one in the same. for example, in the logstash-input-syslog stopping involves closing sockets. and that is what is being done in close as well.

[jsvd]

In some plugins stop will not make run abort right away (maybe stop? will
only be checked in the next loop).
Such plugins might still need to perform cleanup/teardown actions

On Thu, Sep 17, 2015, 20:27 Tal Levy [email protected] wrote:

why not get rid of close/teardown in favor of just using stop for both
stop signalling and cleanup?

—
Reply to this email directly or view it on GitHub
#3895 (comment).

[andrewvc]

@jsvd Is there a reason output plugins don't have stop and stop?. I could use this for the rabbitmq output where it sometimes sleeps if the external rabbitmq server is down, waiting to retry (it could check stop? in a loop.

[talevy]

@jsvd, do you know what is up with this?

https://github.com/elastic/logstash/blob/feature/oob_plugins_stop/lib/logstash/pipeline.rb#L103-L106

  def wait_inputs
    @input_threads.each(&:join)
  rescue Interrupt
    # rbx does weird things during do SIGINT that I haven't debugged
    # so we catch Interrupt here and signal a shutdown. For some reason the
    # signal handler isn't invoked it seems? I dunno, haven't looked much into
    # it.
    shutdown
  end

I think we should clean that up for this PR. not sure what it is referencing.

[jsvd]

Agreed, looking at that comment we can actually reduce the method to:

def wait_inputs
  @input_threads.each(&:join)
end

[jsvd]

@andrewvc eventually both filters and outputs might also have stop. The first implementation of this PR actually added stop/stop? at the LogStash::Plugin level, but only wired the calls for the input plugins, they weren't called during the shutdown.
At the time I could not find a good reason to justify changing the shutdown sequence further and have those methods also in filters/outputs.
I haven't check the rabbitmq plugins, is it really necessary to have those in order to shutdown?

[andrewvc]

@jsvd not necessary, but it would be convenient vs. setting up my own @stopping variable

[jordansissel]

LGTM; didn't run the tests, though.

[colinsurprenant]

WRT to the new do_close and do_stop, I am +1 on the change but I would like to suggest that we do not provide an noop implementation of close and stop in the base class, it is useless IMO and plugin developers do no read base classes, they follow examples and documentation.

instead I suggest we do something like

def do_close
  # do whatever should be done prior to call the real plugin defined close
  close if respond_to? :close
end

same idea with stop