Thread leaks

[GoogleCodeExporter]

Reloading a pircbot results in 2 leaked threads, as the ThreadedListenerManager 
class creates a cached thread pool executor service, but never shuts it down.

I encounter this problem when I reload the IRC module of my application.

Original issue reported on code.google.com by quick.wa...@gmail.com on 18 Dec 2012 at 9:01

[GoogleCodeExporter]

What exactly do you mean by "reloading"?

Original comment by entityreborn on 21 Dec 2012 at 4:46

[GoogleCodeExporter]

I'd assume creating a completely new instance of PircBotX. I believe he's 
saying the ThreadedListenerManager still has a thread going preventing the GC 
from correctly running

Original comment by Lord.Qua...@gmail.com on 21 Dec 2012 at 4:48

• Changed state: Accepted

[GoogleCodeExporter]

Additionally, I'd be tickled pink if you could give me a SSCCE that reproduces 
the issue, to troubleshoot better.

Original comment by entityreborn on 21 Dec 2012 at 4:55

[GoogleCodeExporter]

@quick, what is your setup? How can I reproduce this? We need more information. 
If you could give us a simple self contained compilable example, that'd be 
great.

Original comment by entityreborn on 21 Dec 2012 at 8:16

[GoogleCodeExporter]

@Lord, I'm contemplating keeping a List<Thread> in the ListenerThreadFactory, 
then calling thread.interrupt() when disconnect() is called. Would there be a 
better place/way to do this? I don't know under what conditions he is 
'reloading' his instance, or if he just means reconnecting.

Original comment by entityreborn on 21 Dec 2012 at 8:19

[GoogleCodeExporter]

I was about to close this as "Works for me" but I found one improvement: 
Setting allowCoreThreadTimeOut(true) might solve your problem in  Revision 
c476ca2fe652. Can you try the latest snapshot?

Just FYI, I went in with a debugger before the change and saw that all the pool 
threads time out after 60 seconds. So I'm not sure what your problem is.

Comment again if you have any issues and I'll help you as much as I can

Original comment by Lord.Qua...@gmail.com on 24 Dec 2012 at 6:45

• Changed state: Fixed

[GoogleCodeExporter]

Sorry for the late answer, the notifications went to the wrong email address 
and I was busy with other stuff.
How to reproduce the problem?

1. run: while (true) new PircBotX().connect(...).shutdown();
2. attach a profiler
3. watch the thread count

the problem is, that you create a ExecutorService for the bot right at 
instantiation of the ThreadedListenerManager and never shut it down, so the 2 
threads of the service keep alive until the application stops.
In my case it's a module of the application that get's disabled or reloaded.
I'd suggest adding a shutdown method to ListenerManagers which would be called 
from PircBotX.shutdown()

Original comment by quick.wa...@gmail.com on 24 Dec 2012 at 7:40

[GoogleCodeExporter]

basicly: PircBotX.shutdown() -> ListenerManager.shutdown() -> 
ExecutorService.shutdown()

Original comment by quick.wa...@gmail.com on 24 Dec 2012 at 7:42

[GoogleCodeExporter]

How about this: I added a new method in Revision 34c66ac150d6 called shutdown() 
to only ThreadedListenerManager.

The reason is that I've come to the opinion that ListenerManagers are a users 
responsibility. They are so generic (a good thing) that it makes it difficult 
to write good shutdown code. What if the ListenerManager is tied to another 
bot? What if they didn't use MultiBotManager (where I could solve the first 
question)? Maintaining a list of bots in the ListenerManager seems needlessly 
complex and brings up single responsibility principle issues. 

I think this provides the simplest solution for most users but allowing those 
with more complex bots to still do what they need to do.

Original comment by Lord.Qua...@gmail.com on 24 Dec 2012 at 8:20

[GoogleCodeExporter]

So I have to call ThreadedListenerManager.shutdown() myself?

Original comment by quick.wa...@gmail.com on 24 Dec 2012 at 8:29

[GoogleCodeExporter]

Yes, since its up to the user to manage the ListenerManager. Generally 
shutdown() shouldn't be necessary, but since you need it, its there for you

Original comment by Lord.Qua...@gmail.com on 24 Dec 2012 at 8:30

[GoogleCodeExporter]

Hmm, I actually wouldn't even use the ThreadedListenerManager here, but the 
PircBotX creates one whether or not I want it. There could be a second 
constructor that takes a ListenerManager to be able to use a custom one without 
creating the initial ThreadedListenerManager instance.

Original comment by quick.wa...@gmail.com on 24 Dec 2012 at 8:42

[GoogleCodeExporter]

I have historically been very nervous with adding new constructors as I don't 
want to have a crapton of overloads that setter chains fix. I think for now 
even creating a ThreadedListenerManager that you aren't going to use isn't that 
expensive and will simply be GC'd later.

Original comment by Lord.Qua...@gmail.com on 24 Dec 2012 at 9:02

[GoogleCodeExporter]

You could RAII it, and check if the manager is null in getListenerManager, and 
if it is, create it there. Lazy loading, ftw. Then, people could 
setListenerManager if they want, without creating useless resources.

Original comment by entityreborn on 24 Dec 2012 at 9:08

[GoogleCodeExporter]

Hmm, that's actually a good idea. Added in Revision b1c2816099e5.

Alright, so
 - Allow core pool threads to time out (what ones there were)
 - New method shutdown() that the user must call themselves if they need it
 - Lazy loaded default ListenerManager so its not unnecessarily created when custom ones are used instead

Any more suggestions?

Original comment by Lord.Qua...@gmail.com on 24 Dec 2012 at 9:22

[GoogleCodeExporter]

lazy loading sounds good

Original comment by quick.wa...@gmail.com on 24 Dec 2012 at 10:04