shutdown refactor issues

[colinsurprenant]

there are issues that need to be fixed related to the shutdown refactor.

• a stop method should be implemented that force-closes the socket to make sure it will not ne stuck in IO somewhere regardless if we are using select... for safety.
• I would change the current close method to

def close
  @udp.close rescue nil
end 

and add an identical stop method

def stop
  @udp.close rescue nil
end 

note that with a proper stop the current close method is useless, unless we move the UDPSocket creation in the register method as proposed in #10 where we could have a situation where the plugin register method is called but not the run & stop and then only close, like in specs in which case both close and stop are required.

• there is an error in the spec_helper.rb at

  logstash-input-udp/spec/spec_helper.rb

  Line 17 in 1205076

  plugin.stop

plugin.stop should be rewritten as

plugin.do_stop
input_thread.join

otherwise the stop? method will never return true and there could be race conditions with the input thread.

[ph]

good finding, I understand your points into moving the code into the register and good catch for the shared helpers.

[jordansissel]

+1 on on moving socket setup + ivar stuff into register.

[jsvd]

+1 on the better close method but I don't understand the need to have stop close the socket. Every second select times out and we can check stop?

As to moving the code to register it will help make tests more deterministic, but otherwise improving the plugin itself (and there's tons to do) is out of scope for the shutdown refactoring, IMO

[jordansissel]

@jsvd I agree, we shouldn't need both. Even if we are programming defensively, IO.select should be enough, and if it's not, then something is reaaaaaly wrong with the system (major java/jruby/system bugs) and we can't save ourselves by closing a socket ;P

[jordansissel]

@colinsurprenant curious, why both IO.select and socket closing for interrupts?

[colinsurprenant]

@jsvd who is suggesting to «otherwise improving the plugin itself (and there's tons to do) » ?

[colinsurprenant]

@jordansissel because I have seen too many instances of select not working the way it was expected to work and adding a possible redundant stop to force close the socket is purely defensive, costs nothing to add and has zero risk.

also do not confuse stop and close purposes. In what I suggest it does make sense to have both IMO. but if we decide that we 100% trust select then, yes, stop is not required and we can cross fingers.

[jordansissel]

@colinsurprenant fair, though I think defensively programming against instabilities in the underlying platform really means we should fix the bugs in the underlying platform (jruby). I agree with you on the specific need for defensive programming against select() for now.

[jsvd]

@colinsurprenant it's quite tempting to fix a lot of problems on this plugin, it's what I ended up doing in #10 so I guess I was trying to convince myself :D

I updated #10 to reflect your points.

[jsvd]

@colinsurprenant having given the refactor a try, I ended up rewriting most of the plugin, for a few snowball reasons:

• moving socket creation to register breaks the plugin behaviour of restarting itself so..
• moving socket creation to a method that gets called in both register and during rescue implies changing the places where the @udp socket is closed/cleaned up/etc
• that makes you realize that this plugin, upon restart, does not close the inputworker threads or ensure the @input_to_worker queue (by default holds up to 2000 events) is empty and overwrites it
• (mind explodes).

The other possible route here is to keep it simple and dumb: implement stop/close as you said, but introduce a sleep in the spec suite to raise the odds of run binding the socket before the tests starts writing to it. thoughts?

[jsvd]

Tried to reduce the scope of a pr to just fixing what's broken regarding the shutdown sequence. please review #12
I'll address the other concerns of #9 in another PR