-
Notifications
You must be signed in to change notification settings - Fork 76
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
refactor plugin shutdown #8
refactor plugin shutdown #8
Conversation
end | ||
|
||
def stop | ||
super |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not needed anymore, right?
7b902ac
to
1f95e49
Compare
while !stop? | ||
set_client_socket(TCPSocket.new(@host, @port)) | ||
|
||
if @ssl_enable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered extracting this block into a separate method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could I guess, and a lot of code could be better refactored here too. tried to keep it to minimal.
Are these specs passing for you @colinsurprenant ? I'm currently seeing them hang indefinitely. The code looks alright otherwise. |
is it running agaist the lastest core code base? I've run the tests in a loop to catch potential timing issues and it always passed... weird. |
|
||
# close all sockets and make sure there is not more pending threads | ||
sockets.each{|socket| socket.close} | ||
it "should receive events been generated" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This spec is where it hangs for me
@colinsurprenant It hangs for me here https://github.com/logstash-plugins/logstash-input-tcp/pull/8/files#r40319932 This is running of |
Running with --debug I get it repeating the INFLIGHT EVENTS REPORT message
That's where it hangs |
@colinsurprenant AHA. I was running the specs against JRuby 9.0.0.0 . It worked when I switched to 1.7.20 . In that case LGTM, but that does not bode well for the future. Any ideas why? |
@andrewvc lots have changed IO wise in 9k, we'll have to investigate. Note that we should certainly test against 9k and report problems in JRuby if we find them. OTOH 9k is definitely not mature enough to consider it for a release soon. |
LGTM, tests passed, tested manually server/client without ssl, everything worked. |
comments, fixes and close new specs styles spec helper for input revert data_timeout comment rescue nil
f1974bb
to
957848d
Compare
squashed, merging. |
refactor for the new shutdown semantic.
note that the specs were not refactored as a whole be new specs were added, using the new style, based on the udp input specs. some TODOs have been commented to note some duplicate code and DRYing that we could do in a followup issue/PR.