Refactor plugin shutdown #59

ph · 2015-09-25T15:58:59Z

Gracefully stop the lumberjack server and make sure any frame in transit
will be resend.

ph · 2015-09-25T16:00:21Z

@jsvd a lot less of code to review.

jsvd · 2015-09-29T15:28:22Z

lib/logstash/inputs/lumberjack.rb

      # Wrappingu the accept call into a CircuitBreaker
      if @circuit_breaker.closed?
        connection = @lumberjack.accept # Blocking call that creates a new connection

        invoke(connection, @codec.clone) do |_codec, line, fields|
+          if stop?
+            connection.close


shouldn't this connection.close operation happen in bookkeeping? like:

def close @connection.close end

@lumberjack.close is already done in def close which close the server socket.
connection.close stop reading from the client socket, this allow a bit more graceful shutdown.

See
https://github.com/logstash-plugins/logstash-input-lumberjack/pull/59/files#diff-1b9590423b15f04f215635164e7376ecR101

actually only in def stop, but I guess the argument is valid :)

Oups yes stop , copy paste the other argument ;)

ph · 2015-10-08T14:42:52Z

@jsvd did I have answered all your concerns here?

jsvd · 2015-10-09T07:49:18Z

tests pass but a "real life" test using this pr, ruby-lumberjack master and logstash 2.0 receiving events from logstash 1.5.4 failed:


2015-10-08T21:52:52.123Z Joaos-MBP.lan Hello world!
2015-10-08T21:52:52.123Z Joaos-MBP.lan Hello world!
2015-10-08T21:52:52.123Z Joaos-MBP.lan Hello world!
2015-10-08T21:52:52.123Z Joaos-MBP.lan Hello world!
^C2015-10-08T21:52:52.123Z Joaos-MBP.lan Hello world!
2015-10-08T21:52:52.123Z Joaos-MBP.lan Hello world!
SIGINT received. Shutting down the pipeline. {:level=>:warn}
2015-10-08T21:52:52.123Z Joaos-MBP.lan Hello world!
2015-10-08T21:52:52.123Z Joaos-MBP.lan Hello world!
2015-10-08T21:52:52.123Z Joaos-MBP.lan Hello world!
Lumberjack input: unhandled exception {:exception=>#<LocalJumpError: unexpected return>, :backtrace=>["org/jruby/RubyProc.java:271:in `call'", "/Users/joaoduarte/new_plugins/logstash-input-lumberjack/lib/logstash/inputs/lumberjack.rb:111:in `invoke'", "org/jruby/RubyProc.java:271:in `call'", "/Users/joaoduarte/projects/ruby-lumberjack/lib/lumberjack/server.rb:323:in `data'", "/Users/joaoduarte/projects/ruby-lumberjack/lib/lumberjack/server.rb:305:in `read_socket'", "/Users/joaoduarte/projects/ruby-lumberjack/lib/lumberjack/server.rb:247:in `data_field_value'", "/Users/joaoduarte/projects/ruby-lumberjack/lib/lumberjack/server.rb:158:in `feed'", "/Users/joaoduarte/projects/ruby-lumberjack/lib/lumberjack/server.rb:263:in `compressed_payload'", "/Users/joaoduarte/projects/ruby-lumberjack/lib/lumberjack/server.rb:158:in `feed'", "/Users/joaoduarte/projects/ruby-lumberjack/lib/lumberjack/server.rb:298:in `read_socket'", "/Users/joaoduarte/projects/ruby-lumberjack/lib/lumberjack/server.rb:283:in `run'", "/Users/joaoduarte/new_plugins/logstash-input-lumberjack/lib/logstash/inputs/lumberjack.rb:110:in `invoke'", "org/jruby/RubyProc.java:271:in `call'", "/Users/joaoduarte/projects/logstash/vendor/bundle/jruby/1.9/gems/concurrent-ruby-0.9.1-java/lib/concurrent/executor/executor_service.rb:515:in `run'", "Concurrent$$JavaExecutorService$$Job_290572486.gen:13:in `run'"], :level=>:error}
Lumberjack input: unhandled exception {:exception=>#<NoMethodError: undefined method `run' for nil:NilClass>, :backtrace=>["/Users/joaoduarte/new_plugins/logstash-input-lumberjack/lib/logstash/inputs/lumberjack.rb:110:in `invoke'", "org/jruby/RubyProc.java:271:in `call'", "/Users/joaoduarte/projects/logstash/vendor/bundle/jruby/1.9/gems/concurrent-ruby-0.9.1-java/lib/concurrent/executor/executor_service.rb:515:in `run'", "Concurrent$$JavaExecutorService$$Job_290572486.gen:13:in `run'"], :level=>:error}
Logstash shutdown completed

ph · 2015-10-09T13:19:21Z

1.5.4 was using the bundled version of the input or the master branch?

Gracefully stop the lumberjack server and make sure any frame in transit will be resend.

When we are in the process of closing the server `lumberjack#accept` can return a nil object because now the call is non blocking we have to check if have an actual connection before invoking it.

jsvd · 2015-10-09T14:08:00Z

@ph bundled version. is that reason enough to trigger these bugs on the server side?

ph · 2015-10-09T14:15:34Z

Seem to be in the library, joy

2015-10-09T14:13:34.275Z sashimi 9029
Lumberjack input: unhandled exception {:exception=>#<LocalJumpError: unexpected return>, :backtrace=>["org/jruby/RubyProc.java:271:in `call'", "/Users/ph/es/logstash-plugins/logstash-input-lumberjack/lib/logstash/inputs/lumberjack.rb:111:in `invoke'", "org/jruby/RubyProc.java:271:in `call'", "/Users/ph/es/ruby-lumberjack/lib/lumberjack/server.rb:323:in `data'", "/Users/ph/es/ruby-lumberjack/lib/lumberjack/server.rb:305:in `read_socket'", "/Users/ph/es/ruby-lumberjack/lib/lumberjack/server.rb:247:in `data_field_value'", "/Users/ph/es/ruby-lumberjack/lib/lumberjack/server.rb:158:in `feed'", "/Users/ph/es/ruby-lumberjack/lib/lumberjack/server.rb:263:in `compressed_payload'", "/Users/ph/es/ruby-lumberjack/lib/lumberjack/server.rb:158:in `feed'", "/Users/ph/es/ruby-lumberjack/lib/lumberjack/server.rb:298:in `read_socket'", "/Users/ph/es/ruby-lumberjack/lib/lumberjack/server.rb:283:in `run'", "/Users/ph/es/logstash-plugins/logstash-input-lumberjack/lib/logstash/inputs/lumberjack.rb:110:in `invoke'", "org/jruby/RubyProc.java:271:in `call'", "/Users/ph/es/logstash/vendor/bundle/jruby/1.9/gems/concurrent-ruby-0.9.1-java/lib/concurrent/executor/executor_service.rb:515:in `run'", "Concurrent$$JavaExecutorService$$Job_310320422.gen:13:in `run'"], :level=>:error}
2015-10-09T14:13:34.276Z sashimi 9030

ph · 2015-10-09T14:47:32Z

Too many block makes your head go boooooom! The problem and the fix is really simple. 🌴
To test this make sure you have continuous event coming into logstash.

Since the execution is in a block and the parent is also a block and blocks works like Proc and not lambda (which behave correctly with a return). If the proc you are calling is doing a return the parent function need to return something too if it doesn't you will get the LocalJump error this is related to how scoping work I believe. I havent investigated much

Two way of fixing this:

using break (this is what I did) which will use the default value value returned by the block.
make change to the invoke method to do a return block.call

ph · 2015-10-09T14:50:22Z

@jsvd this should fix the problem you had before. 💃

jsvd · 2015-10-12T11:11:30Z

LGTM

ph added the shutdown semantics label Sep 25, 2015

jsvd mentioned this pull request Sep 25, 2015

Adapt plugins to new Plugin API introduced by #3210 elastic/logstash#3813

Closed

43 tasks

ph assigned jsvd Sep 25, 2015

ph mentioned this pull request Sep 25, 2015

Add support for json data frames #57

Closed

jsvd reviewed Sep 29, 2015
View reviewed changes

ph added 2 commits October 9, 2015 09:58

Refactor plugin shutdown

8b9da72

Gracefully stop the lumberjack server and make sure any frame in transit will be resend.

Fix issue when closing the connection

c19a5ef

When we are in the process of closing the server `lumberjack#accept` can return a nil object because now the call is non blocking we have to check if have an actual connection before invoking it.

ph force-pushed the fix/correct-shutdown branch from c57e24d to 6dba6e1 Compare October 9, 2015 14:48

Break the loop instead of returning which break caller procs

1173a0a

ph force-pushed the fix/correct-shutdown branch from 6dba6e1 to 1173a0a Compare October 9, 2015 14:48

jsvd merged commit 1173a0a into logstash-plugins:master Oct 12, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor plugin shutdown #59

Refactor plugin shutdown #59

ph commented Sep 25, 2015

ph commented Sep 25, 2015

jsvd Sep 29, 2015

ph Sep 29, 2015

jsvd Sep 29, 2015

ph Sep 29, 2015

ph commented Oct 8, 2015

jsvd commented Oct 9, 2015

ph commented Oct 9, 2015

jsvd commented Oct 9, 2015

ph commented Oct 9, 2015

ph commented Oct 9, 2015

ph commented Oct 9, 2015

jsvd commented Oct 12, 2015

Refactor plugin shutdown #59

Refactor plugin shutdown #59

Conversation

ph commented Sep 25, 2015

ph commented Sep 25, 2015

jsvd Sep 29, 2015

Choose a reason for hiding this comment

ph Sep 29, 2015

Choose a reason for hiding this comment

jsvd Sep 29, 2015

Choose a reason for hiding this comment

ph Sep 29, 2015

Choose a reason for hiding this comment

ph commented Oct 8, 2015

jsvd commented Oct 9, 2015

ph commented Oct 9, 2015

jsvd commented Oct 9, 2015

ph commented Oct 9, 2015

ph commented Oct 9, 2015

ph commented Oct 9, 2015

jsvd commented Oct 12, 2015