InputThread#run() should understand SocketException

devBuzzy / pircbotx

Automatically exported from code.google.com/p/pircbotx

0 stars 0 forks source link

Unable to cause a clean quit / disconnection / shutdown / cleanup. I think InputThread class should understand and respect SocketException when calling BufferedReader#readLine() in the surrounding try/catch. It is also allowed for the server to just close the socket (say when shutting down and resetting the client) which also causes SocketException. It should especially be less noisy about an application bug that threw an exception, when the QUIT command has been sent to the server for this client connection. Once the QUIT command has been sent on the wire such an exception is expected. This kind of exception (SocketException) is not recoverable and an expected/anticipated matter that should be handled more gracefully but the project. The same point maybe true for OutputThread#run() while it is waiting for a new queue item or trying to write an item to the socket. What I mean by this is that when the InputThread observes an unrecoverable SocketException and closes the socket it should send an InterruptException to the OutputThread to make it exit cleanly immediately. In addition to this I think the both the InputThread and OutputThread should be marked a Thread#setDaemon(true) so that the JVM process is never kept alive because these threads continue to be alive, i.e. it would be expected that the bot has at least one other control thread, but it is possible to have called quit() and disconnect() and dispose() and have the main thread exit and the JVM continue to remain alive because the InputThread/OutputThread are still alive. I think that dispose() should not return until it has verifiability ensured all such resources have been destroyed. This means signalling (via Thread#interrupt() an then waiting for) the death of all additional threads that have been created by the project. It would also be good to put a label on the threads created Thread#setName("Pircbotx-InputThread-<host>:<port>"). Also during the connect() methods it is possible there is already a socket exception problem that occurs before the method returns. It should check/detect this and perform its own cleanup and indicate failure.

Wow, there are many different parts of your issue. Lets break it into a list 1) "I think InputThread class should understand and respect SocketException when calling BufferedReader#readLine() in the surrounding try/catch. " And do what? I would also like to say that in all the testing I've done I don't think I ever got an unexpected SocketException. 2) "It is also allowed for the server to just close the socket (say when shutting down and resetting the client) which also causes SocketException. It should especially be less noisy about an application bug that threw an exception, when the QUIT command has been sent to the server for this client connection. Once the QUIT command has been sent on the wire such an exception is expected." This was fixed in Issue #24. A quick test shows that it still quits with no Exception printed. Are you sure your running the latest version? If your still running into this issue, post the log and the IRC server and I'll take a look 3) "The same point maybe true for OutputThread#run() while it is waiting for a new queue item or trying to write an item to the socket. What I mean by this is that when the InputThread observes an unrecoverable SocketException and closes the socket it should send an InterruptException to the OutputThread to make it exit cleanly immediately." OutputThread sort of does this already. It actually checks to make sure that the bot is still connected before sending any line to the server through the protected #failIfNotConnected() method. This was also fixed in Issue #24 4) "In addition to this I think the both the InputThread and OutputThread should be marked a Thread#setDaemon(true) so that the JVM process is never kept alive because these threads continue to be alive, i.e. it would be expected that the bot has at least one other control thread, but it is possible to have called quit() and disconnect() and dispose() and have the main thread exit and the JVM continue to remain alive because the InputThread/OutputThread are still alive. I think that dispose() should not return until it has verifiability ensured all such resources have been destroyed. This means signalling (via Thread#interrupt() an then waiting for) the death of all additional threads that have been created by the project." This was discussed in the mailing list ( https://groups.google.com/group/pircbotx/browse_thread/thread/b98c161d8b4824b5 ). Essentially this cannot be done as the InputThread and OutputThread are the ONLY active threads in most basic bots (your main thread exits after you connect and do basic setup). This means that as soon as you connect the JVM exits, which is bad. There was a bug however that I fixed in the latest snapshot about InputThread and OutputThread never closing down correctly. With that fix the JVM can properly exit at the right time 5) "It would also be good to put a label on the threads created Thread#setName("Pircbotx-InputThread-<host>:<port>")." I choose a better bot[num]-Input and bot[num]-Output format since there could be multiple connections to the same server and that your format doesn't show how many bots were created. If you don't like this format then its easily overridable in PircBotX#createOutputThread and PircBotX#createInputThread 6) "Also during the connect() methods it is possible there is already a socket exception problem that occurs before the method returns. It should check/detect this and perform its own cleanup and indicate failure." Indicating failure is done by an exception. Exceptions are the best option here as it shows "Hey, this failed" Cleaning up internal state after the connect method fails halfway through is honestly something I didn't think of. I'll add some code that can fix that

First thank you for your rapid response.

I am using the version 1.6 from Maven.

Maybe you are already maintaining a more recent release at:
https://oss.sonatype.org/content/repositories/snapshots/org/pircbotx/pircbotx/1.
7-SNAPSHOT/

However the javadoc/sources are not being updated with every deploy (they 
should have the same timestamp and version number as each JAR and POM), I guess 
a tweak to the command/method used to do that is all that is needed.

It looks like Issue #24 was resolved in July 2011 and the 1.6 release made in 
Oct 2011 so I guess my information in the original ticket is still valid.

1)

I don't see how all your testing qualifies the response regarding 
SocketException.   But the longer explaining on this (and maybe other points) 
is at the bottom of this text.

2)

No this does not appear to be fixed.  When running an application that connects 
to freenode and I use the QUIT and the disconnect() and then dispose().  I 
still have 2 threads running.  The InputThread and the OutputThread.  When I do 
the disconnect() I see the kind of exception in the output log as reported in 
the Issue #24.

3)

I have not looked at the failIfNotConnected() method but this smells like a 
race condition to me.  The best way to tell if the socket is or is not 
connected is to just send data to it and let the JVM raise the SocketException 
as the error.  That is all that counts.  I call it a race since it sounds like 
(I have not looked at this method) you do some kind of checking of some flag 
and if that is ok you then send some data to the socket, but there is no 
multi-threading synchronization locking taking place.  So there is always a 
window of time/opportunity for another thread to change the flag between the 
time you checked it and the time you send data to the socket.

4) 

Ok I had thought about this possibility (i.e. no Main thread exists) so really 
then I think this feature request would just be a boolean flag changeable via 
setter/getter in PircbotX to setup before calling connect() (and if you call 
the setter after calling connect() you get RuntimeException thrown).  This 
boolean flag turns on or off the Daemonization of InputThread.

I think without question the OutputThread should always be a Daemon thread 
(unless you decide to implement one day some flushing of the output buffer 
always before closing the thread even when the bot is told to shutdown and has 
decided not to accept any further input).  But if you do this you can 
unDaemonize it in the shutdown proceedings, if you know there is worked to do.  
This is the kind of thing Socket#shutdownInput() and Socket#shutdownOutput() is 
for like a HTTP server would do.  But really this is correctness but overkill 
for IRC client (for my purposes anyway).

To maintain compatiblity you make the setting ensure InputThread is non-Daemon 
like it is already.  Then users who want it Daemonized can ensure to call the 
appropriate setter before calling connect().

I shall take a look at the 1.7-SNAPSHOT then with regards to shutting down of 
the threads issue.

5) 

Sure I agree on the throwing exception from connect(), but you must cleanup the 
resources you created that have side-effects that the caller should not need to 
know about.  Such as any threads created, any file handles or sockets handles 
opened.

This is part of my assertion that dispose() should guarantee and enforce that 
all threads have terminated.  The purpose of this method is to ensure all 
resources that have side-effects the PricbotX object created/manages are 
destroyed.

On a general note re issue #24 fixing things...

You should be looking at it from the other angle... this is my thinking.  
followed by my answers.

Is it valid and possible the JVM might cause such an exception to be raised 
(like when waiting for new data on a socket) ?  Yes it is a documented scenario 
in JDK when using socket I/O.

What response should the PircbotX project have when it observes such an 
exception in InputThread ?   There are no useful courses of action that 
PircbotX can take (from inside InputThread) that would be able to recover it 
from this situation, so it should consider the socket handle as being now 
unusable.

So if it is unusable what is the purpose of InputThread ?  To perform blocking 
read I/O and then subsequent data processing on the socket it is charged with 
managing.

So if it can't do this anymore what should it do?  The thread should terminate.

I agree the way to observe the problem is to throw an Exception and the JVM 
does, except the method PircBotX#logException(Throwable) does not look like it 
re-throws it.  It prints an error message and then eats the exception, so there 
is now no way programatically for the application program to get the exception 
to take action.

So this is in the category of the "exception is eaten" anti-pattern.  If it 
re-threw the exception then the loop would abort and cause the thread to exit.  
Since the threads only purpose in life is to service one half (as in 
communication direction) of a socket, then if the one and only handle it does 
the blocking I/O is no longer valid that thread should die it has no purpose 
anymore.

InputThread#run()

...
                        try {
                                line = breader.readLine();
                        } catch (SocketException e) {          // NEW LINE
                                bot.logException(e);           // NEW LINE
                                throw e;                       // NEW LINE
                        } catch (InterruptedIOException iioe) {
                                // This will happen if we haven't received anything from the server for a while.
                                // So we shall send it a ping to check that we are still connected.
                                bot.sendRawLine("PING " + (System.currentTimeMillis() / 1000));
                                // Now we go back to listening for stuff from the server...
                                continue;
                        } catch (Exception e) {
                                bot.logException(e);
                        }
...

I would leave it upto the main PircBotX object to handle the socket.close() 
this resource cleanup is not this threads concern, its concern is I/O reading.  
If anything a socket.shutdownInput() might be tried in a best-effort manner on 
the socket, meaning if it throws an exception then eat the exceptions silently.

Think about where the socket was created/opened, so the responsibility for 
closing it should kept with the same object, like at disconnect() and then 
again in dispose().    I would put the whole InputThread#run() method into a 
try {} and move the last 5 lines of code in the method into a finally { } block.

Speaking of dispose() it should interrupt the 2 threads first, wait for their 
death, then close the socket.  Possibly setting all the reference to the now 
defunc objects to null to capture any other design bugs in the library via NPE. 
 The whole point here is 100% robustness of destruction of resources with 
side-effects.

Original comment by dar...@dlmc.co.uk on 12 Apr 2012 at 12:16

First, please try the snapshot version per https://code.google.com/p/pircbotx/wiki/DevVersion . I've pushed out a new shapshot with javadoc and sources as of Revision ab33fe2c254a. Online snapshot javadoc available at http://site.pircbotx.googlecode.com/hg/apidocs/index.html 1) Ok, well how about this: Revision 5e19d9551a7c where any exception when reading is assumed fatal and the bot begins its shutdown procedures? Note that I don't want to restrict it to SocketException as it would kill the Thread prematurely before stuff can be shutdown 2) Try again with the snapshot. Its been fixed, just not integrated into a release. 3) There was already significant amounts of checking (during adding to the queue, the thread acting on it, and sendRawLineNow) but there is a chance of a race condition. Its been minimized in Revision a881bd7b08ef by moving the check inside the synchronized block in sendRawLineNow. For stuff that does sneak through or other exceptions they are now wrapped in a RuntimeException and thrown instead of just logged. This should be more useful for self recovery and any other issues. 4) TMK the daemon exists so that the thread can be marked as not important enough to prevent the JVM from shutting down. However they are important and closing should be done properly (IE disconnect(), dispose(), reset()) instead of randomly. If for some reason your still dead set on them being daemon threads, you can set them as such by overriding createInputThread and createOutputThread 5) I was about to just wrap the whole connect method in a try..catch and in the catch do the disconnect stuff and then rethrow, but I'm still deciding if any failure at all in connect() should be considered catastrophic. I need more time to verify everything before going forward with that. Also note that the way everything is linked dispose() closing the socket kills off everything else. And if you look at the code, if that fails then the threads are forcefully interrupted. --- For the second part, logging the exception and then throwing it doesn't make sense in InputThread#run as the Thread class by default just prints the exception again. Side note, I generally have considered most exceptions in InputThread and OutputThread those that require manual intervention since something is wrong. Hence why most of the time they are just logged instead of tossed around. Think about it: Can't connect, socket unexpectedly closing, things getting interrupted, can't send, can't close the socket, etc are all things that are difficult to handle anagrammatically and usually show something wrong with your code, the server, or the connection. Moving on, the reason most of the important shutdown code is in InputThread is because its going to be the first thread that gets any information about Socket issues since its always waiting for a line from the server. There is no easier and cleaner way to be notified of Socket issues outside of a watchdog thread. When I have some more time though I'm going to refractor the closing code. Right now its all over the place: InputThread, dispose(), reset(), and probably a few others. I really need to sit down and condense this all into 1 method that does the full shutdown sequence which would simplify things Lastly, dispose() closing the socket is the best way to handle things as its the same action that the server would do during disconnect. It allows for graceful shutdown and only when its necessary should the threads be forcefully closed Hope this explains some things.

devBuzzy / pircbotx

InputThread#run() should understand SocketException #71