devBuzzy / pircbotx

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

Too many Threads created when using a custom ExecutorService #65

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
When you instantiate PircBotX it creates an instance of ThreadedListenerManager 
, which in its constructor creates a thread pool.

Then you set your own ExecutorService using setListenerManager() but it's too 
late, the default thread pool has already been created.

Thus if you do:

setListenerManager(new ThreadedListenerManager(myExecutorService)) you find 
yourself with 2 thread pools for example.

IMO you should have a constructor for setting the ListenerManager (and you 
should have an interface for Bot so that it can easily be mocked even with non 
default constructors).

Thanks!

Original issue reported on code.google.com by vmas...@gmail.com on 7 Mar 2012 at 9:24

GoogleCodeExporter commented 9 years ago
Broke out the debugger to check to see if ThreadPool's die gracefully, and they 
do. All the background threads get killed (you may have tried this yourself but 
there was a bug in the thread pool number where it never got incremented, so it 
looked like there was 2 Thread-0's in the same pool when it was actually 
Thread-0's in different pools). It will get GC'd eventually.

The constructor for setting the Bot and ListenerManager doesn't make sense. 
ListenerManager's don't need to know what bot they are a part of, and Bot's 
have their listenermanager set with the setListenerManager method since I don't 
want to have 10 constructors for all of the various things that can be set.

Original comment by Lord.Qua...@gmail.com on 17 Mar 2012 at 7:38

GoogleCodeExporter commented 9 years ago
I don't agree with your decision to mark this issue as invalid. You're creating 
an unnecessary thread pool and that's just bad design. Creating unnecessary 
objects (especially a thread pool and possibly threads) for no reason is never 
a good idea... 

The fact that you allow setListenerManager() shows that the user can set the 
listener manager to use. My use case is simple: I want to use a different 
listener manager than the default one so creating a thread pool to change it 
just after is unnecessary.

i hope you'll reconsider :)

If you don't, so be it! Future users will be able to see this issue at least!

Original comment by vmas...@gmail.com on 17 Mar 2012 at 8:13

GoogleCodeExporter commented 9 years ago
There are many values and objects that are created by default that the user may 
want to replace in the future. Any of the objects that are replaced get GC'd 
and cleaned up.

I'm sorry, but the complexity of creating constructors for all of the different 
default values doesn't make sense when easy to read and easy to use setters do 
just as well with (IMHO) little consequences.  

Original comment by Lord.Qua...@gmail.com on 17 Mar 2012 at 8:34