zD12 / pircbotx

Automatically exported from code.google.com/p/pircbotx
0 stars 0 forks source link

Why is this project using addShutdownHook() #72

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
src/main/java/org/pircbotx/PircBotX.java constructor

The hook is not even doing anything important or useful, it is adding a hook 
everytime a new instance of the main object is created.  The new Thread is a 
HEAVY resource in this context, has not even been assigned a decent Name to 
identify why it exists in a ThreadDump.

Just remove it this is really bad design (to even consider using for the 
purposes coded into the project).  The JVM process is dying who cares if we 
send a QUIT to the IRC server or just let the operating system kernel perform a 
socket level disconnect.  The IRC server does not care.

If it really does need to stay:
 * Allow the user control over the headache that is addShutdownHook() being used by Java libraries making up a larger system.  This feature should not be a default.
 * Use one Thread for the entire PircbotX project that can handle any number of PricbotX object instances being created and destroyed over the lifetime of the JVM process.
 * Use WeakReference to link back to the PircbotX objects being retained so they are not forever pinned into memory.
 * Have the PircbotX object itself manage its own register and unregister with the shutdown hook, in the constructor and the dispose() methods.
 * Move this functionality functionality to a new class and document how a user can themselves add a shutdown hook (from their application code) using one line of code using the implementation provided.

Original issue reported on code.google.com by dar...@dlmc.co.uk on 12 Apr 2012 at 12:27

GoogleCodeExporter commented 9 years ago
First, let me say that this had good intentions. During development I noticed 
several times that the JVM would not close down sockets correctly when 
forcefully shut down meaning the bot was still online on the IRC server. This 
caused issues since while coding I forcefully stopped PircBotX many times and 
on reconnect had issues since the nick was already taken. This was the only way 
I could think of to remedy that situation.

However I'm not going to make the shutdown hook a non-default option. First, 
its been added by default for quite some time and removing it would affect 
backwards compatibility. Second, it is very easy to remove afterwords

I've went ahead and made a batch of changes that should make the shutdown hook 
much more friendly

Revision 689992d22888 - ShutdownHook thread is now tracked instead of randomly 
created with no reference
Revision 3c78c8fb9268 - Added two new methods: hasShutdownHook and 
useShutdownHook. These allow the shutdown hook to be configured and checked. 
Also used WeakReferneces (didn't even think about GC when I originally wrote 
it, sorry about that)
Revision ab33fe2c254a - Gave the shutdown thread a meaningful name.

Things I opted not to do
 - A single shutdown hook that exits all the bots - Tricky, especially when it comes to memory leaks. Also the combined length of shutting down all the sockets might make the JVM kill the hook since it takes too long
 - Moving to a new class - Completely unnecessary and adds to class bloat. useShutdownHook is very easy to use and has documentation. The next feature request would be replacing the shutdownHook with a person's custom one instead of properly adding extra shutdown code by overriding dispose().
 - Configuring in the constructor - I'm not adding any setup to the constructor as there's already a large amount of setup that exists happily without being set in the constructor. The next feature request would be to add other parameters and then you end up with new PircBotX(true, false, false, true, "Some string", new Something(), false, true, "What does this do again?"), which is hardly an idea system. And trying to minimize a single thread being created that isn't going to be used is a premature optimization on any system made within the past 10-15 years. 

Thanks for the bug report

Original comment by Lord.Qua...@gmail.com on 12 Apr 2012 at 1:31