Make usage of the new shutdown api + small rspec refactor

[purbon]

Implements stop to return from run according to elastic/logstash#3210

Depends on elastic/logstash-devutils#32 and elastic/logstash#3812

Fixes #11

[jordansissel]

Not sure this change feels right. It saves us one line of code here, but moves it to another file and requires a new file to be maintained. Can you tell me about the gain here?

[purbon]

Me having the old "automatic" behaviour of pointing everything to a single spec_helper per project, so there we can have specific helpers, mocks, etc. But I understand this does not provide much here 👍

[jordansissel]

The pipe from echo test hangs for me, but may be unrelated to this change. I haven't investigated yet.

The new test doesn't pass for me:

⓿ crinkle(~/projects/logstash-input-pipe) pull/10 !1! 
% bundle exec rspec  -f d -e 'interruptible'
Run options:
  include {:full_description=>/interruptible/}
  exclude {:redis=>true, :socket=>true, :performance=>true, :couchdb=>true, :elasticsearch=>true, :elasticsearch_secure=>true, :export_cypher=>true, :integration=>true, :windows=>true}

LogStash::Inputs::Pipe
  behaves like an interruptible input plugin
    stop
      returns from run (FAILED - 1)

Failures:

  1) LogStash::Inputs::Pipe behaves like an interruptible input plugin stop returns from run
     Failure/Error: wait(1).for { thread }.to be_alive
       expected `#<Thread:0x132ddbab dead>.alive?` to return true, got false
     Shared Example Group: "an interruptible input plugin" called from ./spec/inputs/pipe_spec.rb:14

[jordansissel]

(I have elastic/logstash#3812 and elastic/logstash-devutils#32 installed locally)

[purbon]

Thanks for your test @jordansissel I will investigate this.

[purbon]

If I understand your issue @jordansissel properly, this could happen because the plugin actually finished even before the first check for being alive. This makes me to have a question here, how do you guess this would have happen? might this be an error during the loop? Can you check if you see any kind of exception or logging message?

I will try to reproduce, but I think the situation you see if because I took away https://github.com/logstash-plugins/logstash-input-pipe/blob/master/lib/logstash/inputs/pipe.rb#L66 in this PR, I will bring it back and introduce a way to wakeup the thread if necessary. Hope this provide a solution to your issue.

[purbon]

@jordansissel, this test uses a unicode character, might be there is a problem regarding this?

[purbon]

My expectation is the new test added https://github.com/purbon/logstash-input-pipe/commit/88abb4836da765f6650560581611722d041908c9 should not be failing in your env, can you check that for me?

Thanks a lot.

[jordansissel]

what is the purpose of this? Should we be using a synchronization mechanism instead? I'm not sure I like sleep+wakeup, but wait+notify is more common (a ConditionVariable) and I feel more confident in its thread safety.

[jordansissel]

Both @jsvd and @purbon report the interrupted tests passing for them, so I'm content to believe something is wrong with my environment.

I did a pass at code review. Overall looking good; had some questions about the @sleep stuff and locking and left inline comments.

[colinsurprenant]

unless Stud.interval is used, Stud.stop! is not required.

[purbon]

cool, I got it, I thought it was necessary to stop stoppable_sleep too.

[purbon]

@talevy @jordansissel @colinsurprenant is this plugin ready to be merged?

[suyograo]

@purbon can you rebase?

[talevy]

pre-rebase: LGTM

[elasticsearch-bot]

Merged sucessfully into master!