callbacks registered to Log4jShutdown is not executed when stop is called

[navis]

ShutdownCallbackRegistry#addShutdownCallback is expected to register callback to be executed upon shutdown. But in Log4jShutdown, it's not.

[drcrallen]

Why do you say it is not?

[drcrallen]

I ask because of
#2124
#2123
#2122
being due to lifecycle.stop() not being able to be called multiple times without errors

[drcrallen]

IMHO this is more appropriately solved by metamx/java-util#28

[drcrallen]

I am not opposed to the changes in this file though, as it adds better future proofing of the shutdown process for peons

[drcrallen]

oh I see now. ha! yeah, that needs fixed.

[drcrallen]

Ran into a spark bug a few days ago where a shutdown hook firing during a "clean" shutdown causes problems. This method actually needs synchronized or else a shutdown hook fired from a SIGTERM won't wait for runCallbacks() to finish

[drcrallen]

Note that https://github.com/metamx/java-util/blob/master/src/main/java/com/metamx/common/lifecycle/Lifecycle.java#L267 does synchronize, so it may be sufficient to simply state in a comment on the stop method that synchronization is assumed to be performed in the caller

[drcrallen]

Looks good to me! Thanks for the catch. All my comments are minor and I'm fine if it goes in as is 👍

[navis]

@drcrallen Revised concurrent stop threads to wait completion of shutdown process. Is this better?

[drcrallen]

Can you put a sane timeout on here? like maybe 1 minute or something?

[navis]

done.

[nishantmonu51]

is there a race here ? by the time waitTransition is called the state can be already stopped ?

[nishantmonu51]

nm, I see the method waitTransition returns true if the current state is stopped.

[navis]

met #2128. running test again

[drcrallen]

Since this is using Thread now, can this simply call hook.run()?

[navis]

Could be. But if lifecycle.start is called once, should we be expected to call lifecycle.stop?

[drcrallen]

Yes, hook.run() should call its Runnable.run() which calls lifecycle.stop().

But in the interest of minimizing changes for this PR such a change can be ignored for now.

[drcrallen]

👍 after travis