views:

200

answers:

7

I have a method that looks like this:

try {
  doStuff();
} catch (Exception ex) {
  logger.error(ex);
}

(I don't really use method names like doStuff - this is just to make things easy)

In doStuff I do a variety of things, among them is call a data access method (so, another method within doStuff) that ends with the following:

} catch (SQLException ex) {
  logger.error(ex);
} finally {
  try {
    connection.close();
    proc.close();
    results.close();
  } catch (Exception e) {
    logger.error(e);
  } //<--Exception thrown here. HUH?
}
return stuff;

When stepping through this code I get to the second to last curly brace (marked with a comment) and then jump up to the catch in the first code block with a NullPointer exception. The results.close() is what is being run right before it (results is not null). My IDE (NetBeans) doesn't provide a stack trace (it shows the stack trace is null) or any other information other than the name of the exception (from what I can tell).

This code was running fine previously. In fact while it was running, I changed the stored procedure that the data access method (where I'm getting this exception) was calling, and then this error started to occur (without the application having been stopped at all). I've since tried rebuilding and restarting but to no avail. I could change the sproc back but I really want to find out what this error is from since it makes no sense that the sproc would even be a part of this considering where in the code the exception is occurring.

+2  A: 

It could well be that logger is null.

Hard-to-pinpoint exceptions are often thrown in the exception handler itself.

Eric J.
+1: That was my first thought, too.
R. Bemrose
This was my first thought as well, however, that's not the case. In fact, the logger is actually used within the same method prior to the code in the second block.
Ryan Elkins
+3  A: 

Close the resources in the reverse order in which they were obtained:

try 
{ 
    results.close(); 
    proc.close(); 
    connection.close(); 
} 
catch (Exception e) 
{ 
    logger.error(e); 
} //<--Exception thrown here. HUH? 

I'd also recommend methods like these:

public class DatabaseUtils
{
    // similar for ResultSet and Statement
    public static void close(Connection c)
    {
       try
       {
           if (c != null)
           {
               c.close();
           }
       }
       catch (SQLException e)
       {
           // log or print.
       }
    }
}
duffymo
Thanks for the tip, I'll do that.
Ryan Elkins
you should never catch(Exception e) and always catch a sub-class.
fuzzy lollipop
Can't see why a down vote for this. What could possibly be wrong?
duffymo
+2  A: 

NullPointerException can not be thrown in a line without a statement.

Check that the class file you are executing is of the same version as the source you view (I have had similar issues when an incorrectly configured classpath contained a class twice and the older version was found first in the classpath, or a recompiled class files for not correctly copied to the web container I used for testing).

Edit: As emh points out, it could also be that exception occured prior to entering the finally block.

meriton
not entirely true.in his case if the exception was thrown in doStuff() and there was no catch block to catch it then the execution path would still have to execute the finally block before the exception could be thrown to the caller. so that's why the exception looks like it is being thrown from the closing brace - that's the last line of the finally block.
emh
You're right, will edit.
meriton
+3  A: 

your doStuff() method is throwing something other than a SQLException and it is not being caught. add a catch(Exception e) block and log that exception and see what happens.

this code sample exhibits the same behaviour you are describing:

public class TryCatchTest {
    public static void main(String[] args) {
        try {
            System.out.println("foo");
            throw new NullPointerException();
        } finally {
            try {
                System.out.println("bar");
            } catch (Exception e) {
                e.printStackTrace();
            }
        } // exception thrown here
    }
}
emh
This was it exactly. Thanks.
Ryan Elkins
catching (Exception e) is bad practice, catch the individual expected sub-classes instead
fuzzy lollipop
A: 

I'm 99% sure this is happening in the JDBC driver. For starters, your close statements are backwards. You should close the resultset, the statement and the connection, in that order.

If you are running in an application server which is managing the transactions, then the attempt to commit the transaction may trigger the exception inside the JDBC driver.

It could also be something about how result sets are generated in the stored proceedure, such as accessing one, then accessing another, and then referring back to the first one.

Yishai
A: 

you should NEVER use catch(Exception e) and always catch a particular Exception sub-class, this way you know what is going on where.

fuzzy lollipop
I disagree, but only partially. The tenant to go by is to not catch exceptions you aren't going to deal with, that is, unless you are going to do something else with it.So, if you don't care about what the exception is, just that an exception happened, catch Exception. Just don't make the mistake of catching Exception and assuming it is a particular type (in this case SQLException).In this particular case, I wouldn't catch the exception unless I was going to do something other than log or printStackTrace() .
Jeff Walker
I use catch(Exception ex) when I have no idea what could be thrown or I just want to prevent the application from crashing (or at least log something before it does). In this case it was not catching a specific exception that actually caused the problem.
Ryan Elkins
Also, it is sometimes needed to catch java.lang.Throwable to fully avoid crashing. But of course, use that with caution.
Jeff Walker
A: 

As I said in a comment, never catch an exception that you don't want to deal with. In your code, assuming that it is complete, you are not doing anything interesting with the exception, and it is causing you confusion on where and why the exception is happening. If you really want to do more than log or printStackTrace(), like wrapping it with a domain-specific exception (like your.package.DataAccessException or something), then great.

Do something more like this:


ResultSet results = null;
CallableStatement proc = null;
Connection connection = null;
try {
  connection = >
  proc = connection.createCallableStatement(..);
  results = proc.execute(..);
  >
} finally {
  try {
    if ( results != null ) {
      results.close();
    }
  } catch (Exception e) {
    logger.error(e);
  }
  try {
    if ( proc != null ) {
      proc.close();
    }
  } catch (Exception e) {
    logger.error(e);
  }
  try {
    if ( connection != null ) {
      connection.close();
    }
  } catch (Exception e) {
    logger.error(e);
  }
}

And then in the caller:


    try {
        doStuff();
    } catch ( SQLException e ) {
        throw new your.package.DataAccessException(e);
        // or just let the SQLException propagate upward
    } catch ( Exception e ) {
        throw new your.package.AppException("omg, crazy pills!", e);
        // or, once again, just let the exception
        //  propagate and don't catch anything
    }

So, take-away:

  1. don't log exception where they happen, just pass them on, nested in another exception. You don't want your process to not know whether or not the SQL action succeeded or not. You would rather stop and do something else.
  2. Nest exceptions until the get to the end of the line, that way, you always have the complete trace in the place that you wanted to deal with the exception, not in five places scattered throughout your server log.
  3. Nest exceptions (yes, I said that twice!) so that you don't care where the JVM actually throws the exception from, you have the next exception to follow, telling you it was actually a callable statement, or improper closing of your resources, etc.
  4. Don't nest and throw exceptions from errors caught in your finally code, that will interfere with the original exception and that will be more interesting than the failure to close and statement that didn't get opened in the first place.
  5. Set variables to null before you use them, and then check for null before close()ing them.
  6. Catch each problem in the finally block individually, as you might not be able to close your ResultSet (because some execution error caused it not to open in the first place), but you should still try to close the CallableStatement and Connection, as it is probably unaffected and will still cause you to leak resources.

Hope that helps.

Jeff Walker