elastic / ruby-lumberjack

Ruby lumberjack protocol, contains both client and server
Other
4 stars 17 forks source link

Allow to close a connection and a server #13

Closed ph closed 9 years ago

ph commented 9 years ago

When doing the refactor of the inputs plugin to support a clean shutdown, I saw there was no way to gracefully close a Lumberjack server. This PR add #close method on the server class and the connection class.

ph commented 9 years ago

@jordansissel I have updated this PR with our discussion for passing the context to the Connection class.

jordansissel commented 9 years ago

Code LGTM. Will test shortly.

jordansissel commented 9 years ago

rspec tests pass. doing some manual testing also.

jordansissel commented 9 years ago

Manual tests indicate Server.close hangs for me:

% ruby -I./lib -e 'require "stud/trap"; require "lumberjack/server"; s = Lumberjack::Server.new(:port => 4444, :ssl_certificate => "./lumberjack.crt", :ssl_key => "./lumberjack.key"); q = Queue.new; Kernel.trap("INT") { q << "INT" }; Thread.new { begin; q.pop; p "got interrupt"; s.close; p "s.close complete"; rescue => e; p :e => e; end };  s.run { |x| p x }'

{"file"=>"-", "host"=>"Macintosh.local", "offset"=>"32166", "line"=>"testing"}
^C"got interrupt"
"s.close complete"

The program never terminates (s.run never completes)

I can confirm that at least s.close terminates the client connection because lsf discovers very quickly that the connection has been terminate: 2015/09/23 12:35:42.597328 Read error looking for ack: EOF - good behavior there.

jordansissel commented 9 years ago

Under MRI 2.1.6, Server.close causes Server.run's accept() to raise EBADF:

% ruby -I./lib -e 'require "stud/trap"; require "lumberjack/server"; s = Lumberjack::Server.new(:port => 4444, :ssl_certificate => "./lumberjack.crt", :ssl_key => "./lumberjack.key"
); q = Queue.new; Stud.trap("INT") { q << "INT" }; Thread.new { q.pop; s.close };  s.run { |x| p x }; sleep 5'
{"file"=>"-", "host"=>"Macintosh.local", "offset"=>"32112", "line"=>"hello"}
^C/Users/jls/.rvm/rubies/ruby-2.1.6/lib/ruby/2.1.0/openssl/ssl.rb:286:in `accept': Bad file descriptor (Errno::EBADF)
        from /Users/jls/.rvm/rubies/ruby-2.1.6/lib/ruby/2.1.0/openssl/ssl.rb:286:in `accept'
        from /Users/jls/projects/ruby-lumberjack/lib/lumberjack/server.rb:73:in `accept'
        from /Users/jls/projects/ruby-lumberjack/lib/lumberjack/server.rb:64:in `run'
        from -e:1:in `<main>'
ph commented 9 years ago

@jordansissel Okay, I have added the logic we have discussed, still missing test I will do them shortly.

This is what I did:

What I will also do.

ph commented 9 years ago

Running your script allow this library to gracefully shutdown.

~/e/ruby-lumberjack git:fix/refactor-shutdown ❯❯❯ ruby -I./lib -e 'require "stud/trap"; require "lumberjack/server"; s = Lumberjack::Server.new(:port => 4444, :ssl_certificate => "./lumberjack.crt", :ssl_key => "./lumberjack.key"); q = Queue.new; Kernel.trap("INT") { q << "INT" }; Thread.new { begin; q.pop; p "got interrupt"; s.close; p "s.close complete"; rescue => e; p :e => e; end };  s.run { |x| p x }'
^C"got interrupt"
"s.close complete"
~/e/ruby-lumberjack git:fix/refactor-shutdown ❯❯❯
ph commented 9 years ago

@jordansissel can you do an early realy before I write the test, I would like to get your opinion. Because if this work we can probably use some of the logic in the TCP/SSL handling.

ph commented 9 years ago

@jordansissel I have tested delaying the ssl handshake in the last minute in https://github.com/elastic/ruby-lumberjack/pull/18

Test are passing but I will do more stress testing today.

jordansissel commented 9 years ago

I have tested this branch manually and the prior failure (uninterruptable Server.run) is now resolved. Yay! I'll finish reviewing shortly.

jordansissel commented 9 years ago

Overall code is looking almost ready! Back to you, @ph! :)

jordansissel commented 9 years ago

Tests pass. Code LGTM. @jsvd left a few style notes that I agree with.

I'm OK with this merging. :)

ph commented 9 years ago

Updated with the latest changes merging.

elasticsearch-bot commented 9 years ago

Merged sucessfully into master!