reconnect() returns IrcException when recovering from a disconnect

[GoogleCodeExporter]

Steps:
1. connect(SERVER, PORT)
2. Get disconnected (I used TCPkill)
3. reconnect()

Outcome:
org.pircbotx.exception.IrcException: Cannot reconnect to an IRC server because 
we were never connected to one previously!
    at org.pircbotx.PircBotX.reconnect(PircBotX.java:409)
    at DCListener.onDisconnect(DCListener.java:43)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
    at java.lang.reflect.Method.invoke(Method.java:597)
    at org.pircbotx.hooks.ListenerAdapter.onEvent(ListenerAdapter.java:127)
    at org.pircbotx.hooks.managers.ThreadedListenerManager$1.run(ThreadedListenerManager.java:89)
    at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:441)
    at java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:303)
    at java.util.concurrent.FutureTask.run(FutureTask.java:138)
    at java.util.concurrent.ThreadPoolExecutor$Worker.runTask(ThreadPoolExecutor.java:886)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:908)
    at java.lang.Thread.run(Thread.java:662)

What version of the product are you using? On what operating system?
1.5
Linux (Ubuntu 10.04)
Java 1.6.0_26

Please provide any additional information below.
It appears that on disconnect, _server becomes null. reconnect() throws 
IrcException when getServer() returns null. Trying connect() directly gives a 
java.lang.IllegalThreadStateException.

Original issue reported on code.google.com by mimitheb...@gmail.com on 17 Aug 2011 at 6:28

[GoogleCodeExporter]

I'll have to take a look at it. I know that calling connect() again isn't going 
to work simply because its dangerous to have a single bot connect to multiple 
servers. Although I probably should give a much more informative exception 
(IllegalThreadStateException I think comes from trying to restart the output 
thread, which you can't do, 

Original comment by Lord.Qua...@gmail.com on 21 Aug 2011 at 3:24

• Changed state: Accepted

[GoogleCodeExporter]

There is an exception when attempting to 'jump' to a new server as well.

steps:
1. connect(server,port);
2. quitServer(); or disconnect();
3. connect(newServer,port); (same port or different port)

output:
java.lang.IllegalThreadStateException
    at java.lang.Thread.start(Thread.java:612)
    at org.pircbotx.PircBotX.connect(PircBotX.java:327)
    at org.pircbotx.PircBotX.connect(PircBotX.java:268)
    at ircbot.Bot.connectToServer(Bot.java:131)
        ...

Original comment by jenga...@gmail.com on 26 Aug 2011 at 6:49

[GoogleCodeExporter]

While that exception is the wrong one to throw, it shows what I don't want 
people doing: calling connect() more than once. I'm nervous about allowing 
connect() to be called twice. While it does simplify things when reconnecting 
(maybe you used the wrong server address and you want to go through a list, 
maybe its the wrong port, maybe you need SSL), having a bot connect to a 
different server entirely is not a good thing as there's still state specific 
to the server that you were connecting to. The next bug report I'm going to get 
is from someone trying to use a single bot to connect to completely different 
IRC servers and random parts of PircBotX breaking

What would you recommend doing?
 - Essentially duplicate all the connect() overloads in reconnect() methods. This provides a clear separation between connecting to a server initially and reconnecting later. It does however clutter the method list and doesn't look as pretty
 - Allow connect to be called twice. This has all the issues that I listed above

Original comment by Lord.Qua...@gmail.com on 26 Aug 2011 at 7:04

[GoogleCodeExporter]

I think allowing connect() only if some server, port, and password is not 
globally set is acceptable.

after calling disconnect(), reconnect() should fail, reset() should be called, 
and connect() should work

after calling quitServer(), reconnect() should work, reset() should not be 
called, and connect() should fail

Example:
connect(hostname,port,password,socketFactory){
   if(getServer()!=null) //only allows for one server
     throw new exception
      .....
}

disconnect(){
  reset(); //allows for new connect() to be called
  quitServer();
}

When the bot has a disconnect event (in InputThread.java), the reset() function 
is called and generates the IllegalThreadStateException problem.  I think 
reset() should be called only if the user expects to call connect() again... 
otherwise, we will still need the server and port.

Original comment by jenga...@gmail.com on 26 Aug 2011 at 8:38

[GoogleCodeExporter]

I agree with Leon on the topic of disallowing programmers from calling 
connect() in an attempt to connect the bot to many servers. If the bot is 
already connected, it should throw an IrcException in my opinion.

There are a few scenarios to consider: Trying a given list of servers and 
cycling through them until one gives a successful connect, and recovering from 
a ping timeout or another disconnection. In both cases, if the initial connect 
failed, or if the bot pinged out, the bot is disconnected. Using 
connect(args...) would allow to try the next server on the list, and 
reconnect() would try to reconnect to the last server.

I'm not sure I fully understand Jenga's comment enough to comment on it. As far 
as I know, according to the JavaDoc, disconnect() calls quitServer(). I do 
agree however that an exception should be throw if already connected.

Original comment by mimitheb...@gmail.com on 26 Aug 2011 at 9:23

[GoogleCodeExporter]

disconnect() does call quitServer(), but that is all it calls. That being said, 
disconnect() is a redundant method.  I am suggesting that calling either 
disconnect() or quitServer() should allow for a new connect() call to be made 
(by also calling reset()).

Like you suggested, trying a list of servers along with something like a 
nextServer() and nextServer(String server) method would be great.

Original comment by jenga...@gmail.com on 26 Aug 2011 at 11:28

[GoogleCodeExporter]

I've relented. I didn't think about going through a list of servers or 
connecting to the same server but on a different port, different password, or 
different socket factory.

Revision 7f48ee43792b - reset() doesn't clear server values that are important 
for reconnecting
Revision 92c5ed98a9d9 - Don't attempt to reuse the same OutputThread, which 
caused that IllegalThreadStateException. Doing that was inherited from PircBot, 
however there are some substantial changes in InputThread and OutputThread that 
must of broken this mechanism.
Revision 82e9e7f08c91 - quitServer() now throws a RuntimeException when you've 
already disconnected from the server. Its cleaner than waiting for the 
OutputThread to grab the line off the queue then panic when there's no 
connection

I did some quick tests and they seem to be working. Tell me if something is 
wrong and I'll reopen this issue

Original comment by Lord.Qua...@gmail.com on 27 Aug 2011 at 1:05

• Changed state: Fixed

[GoogleCodeExporter]

It's working perfectly! Thanks!

Original comment by mimitheb...@gmail.com on 28 Aug 2011 at 6:02