tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From J Ross Nicoll <jrn2...@dcs.st-and.ac.uk>
Subject Catching java.lang.Throwable
Date Sat, 12 Aug 2006 12:35:58 GMT
Found this recently: 
http://www.jroller.com/page/fate/?anchor=why_i_hate_tomcat

Most of it is ranting about parts of Tomcat I don't have any experience 
with working on, but it does mention Throwable being caught all over the 
place:

"The first issue is the fact that everywhere, Throwable is caught. Yes, 
even Error type exceptions are caught. Things that god and Sun never 
intended for applications to even try to handle, tomcat will (silently) 
catch and ignore. After all, when you run out of memory, best thing to 
do is keep going right?"

I'm currently working on some patches to reduce the number of places 
that catch Throwable, but felt a little bit of an explanation of what 
why catching Throwable is bad, might be in order. Trying not to make 
this sound too much like a lecture, and hope I don't irk people too much...

Looking at
"connectors/http11/src/java/org/apache/coyote/http11/Http11Processor.java", 
line 812:

try {
     socket.setSoTimeout(soTimeout);
} catch (Throwable t) {
     log.debug(sm.getString("http11processor.socket.timeout"), t);
     error = true;
}

java.net.Socket.setSoTimeout(int) only lists java.net.SocketException as 
an exception it throws, so why are we catching everything? In 
particular, imagine the VM runs out of memory, the GC can't free any 
more up, and java.lang.OutOfMemoryError is thrown. This will happily 
ignore that error, only logging it if debugging level is enabled, and 
continue. While, sure, the alternative may be that Tomcat exits 
abruptly, but really is that any better to Tomcat grinding to an 
unpleasant halt? It's at least easy to tell a server has crashed, but 
unable to do anything because it's run out of memory is much trickier.

Further down the same file, line 838:

// Parsing the request header
try {
     if( !disableUploadTimeout && keptAlive && soTimeout > 0 ) {
         socket.setSoTimeout(soTimeout);
     }
     inputBuffer.parseRequestLine();
     request.setStartTime(System.currentTimeMillis());
     thrA.setParam( threadPool, request.requestURI() );
     keptAlive = true;
     if (!disableUploadTimeout) {
         socket.setSoTimeout(timeout);
     }
     inputBuffer.parseHeaders();
} catch (IOException e) {
     error = true;
     break;
} catch (Throwable t) {
     if (log.isDebugEnabled()) {
         log.debug(sm.getString("http11processor.header.parse"), t);
     }
     // 400 - Bad Request
     response.setStatus(400);
     error = true;
}

I'm fairly certain it should be returning 500, not 400, but that's not 
the point. Here, catching Throwable is correct; you do not want an error 
to leave the connection in an indeterminate state. However, once the 
error status has been sent, it should be thrown upwards again.

It might make sense to catch specific Error subclasses higher up, if 
they can be dealt with (for example, catch OutOfMemoryError, clear out 
caches, and then call the GC to try fixing the situation), but catching 
Throwable throughout the code is dangerous. Particularly, some of the 
errors, for example ThreadDeath, need to propagate upwards to ensure 
correct operation of the VM!

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Mime
View raw message