NullPointerException closing a PreparedStatement in separate thread.

[GoogleCodeExporter]

Through design decisions beyond my control, we access a ResultSet via a 
java.util.Iterator interface.  Because Iterator does not provide a close() 
method, the ResultSet and Statement backing the iterator are closed in a 
finalize() method on my Iterator implementation.   Because of this the 
PreparedStatement is closed in the "Finalizer" thread, not the thread that ran 
the query.

This initially failed with H2 1.2.140, so I upgraded to 1.2.144, but it still 
fails.
We are using H2 in embedded mode, with MVCC=TRUE against a million-row database.
The queries would return most or all of the database - think
  SELECT * FROM table WHERE ( primarykey > 'somevalue' ) ORDER BY primarykey

With that in mind consider this:

Two threads, Thread0 and Thread1, perform similar searches on the database.  
They get connections from an application level ConnectionPool, prepare a query 
statement, run the query, and return an Iterator wrapping the ResultSet.  When 
the finalizer() runs on an abandoned Iterater, the statement is closed and the 
connection is returned to the pool.

I seem to have reduced a failure condition to this:

 - Thread0 gets a connection, runs a query, returns Iterator wrapped ResultSet.
 - The caller processes some subset of the ResultSet and abandons it (for instance, only processes the first n items).
 - Thread2 (in our implementation this is the Finalizer) calls statement.close().
 - Thread1 gets the same connection, prepares a statement, and tries to execute it.
 - executeQuery() throws a NullPointer exception.  Stack trace as follows:

     [java] org.h2.jdbc.JdbcSQLException: General error: "java.lang.NullPointerException" [50000-144]
     [java]   at org.h2.message.DbException.getJdbcSQLException(DbException.java:327)
     [java]   at org.h2.message.DbException.get(DbException.java:156)
     [java]   at org.h2.message.DbException.convert(DbException.java:279)
     [java]   at org.h2.message.DbException.toSQLException(DbException.java:252)
     [java]   at org.h2.message.TraceObject.logAndConvert(TraceObject.java:386)
     [java]   at org.h2.jdbc.JdbcPreparedStatement.executeQuery(JdbcPreparedStatement.java:104)
     [java]   at com.google.enterprise.connector.util.documentstore.LocalDocumentStoreImpl.runQuery(LocalDocumentStoreImpl.java:312)
     [java]   at com.google.enterprise.connector.util.documentstore.LocalDocumentStoreImpl.getDocumentIterator(LocalDocumentStoreImpl.java:286)
     [java]   at com.google.enterprise.connector.util.documentstore.DocStoreBench$BenchDocIdRead.run(DocStoreBench.java:533)
     [java]   at java.lang.Thread.run(Thread.java:655)
     [java] Caused by: java.lang.NullPointerException
     [java]   at org.h2.jdbc.JdbcResultSet.<init>(JdbcResultSet.java:97)
     [java]   at org.h2.jdbc.JdbcPreparedStatement.executeQuery(JdbcPreparedStatement.java:101)
     [java]   ... 4 more

This blows up in the JdbcResultSet constructor, at the line:
        setTrace(conn.getSession().getTrace(), TraceObject.RESULT_SET, id);

So either conn is null (doubtful) or conn.getSession() returns null.

I instrumented the code.  When I look at the logs, the typical call to 
getConnection returns a new connection (when printing it out the connection 
name is "conn0", "conn1", "conn2", ...)  However, occasionally, getConnection 
returns a connection that was just returned to the pool, so the returned 
connections would be "conn0", "conn1", "conn2", "conn1".   When that happens - 
NPE.

Considering that behavior, I examined my ConnectionPool logic:

  // Return a Connection from the ConnectionPool.  If the pool is empty,                                                                                                        
  // then get a new Connection from the DataSource.                                                                                                                             
  public synchronized Connection getConnection() throws SQLException {
    Connection conn;
    try {
      // Get a cached connection, but check if it is still functional.                                                                                                          
      do {
        conn = connections.remove(0);
      } while (isDead(conn));
    } catch (IndexOutOfBoundsException e) {
      // Pool is empty.  Get a new connection from the dataSource.                                                                                                              
      conn = dataSource.getConnection();
    }
    return conn;
  }

  // Release a Connection back to the pool.                                                                                                                                     
  public synchronized void releaseConnection(Connection conn) {
    connections.add(0, conn);
  }

  // Returns true if connection is dead, false if it appears to be OK.                                                                                                          
  private boolean isDead(Connection conn) {
    try {
      conn.getMetaData();
      return false;
    } catch (SQLException e) {
      close(conn);
      return true;
    }
  }

Examining the logs, I am regularly returning Connections to the pool, but 
getConnection() seems to almost always return new Connections.  Theoretically, 
since I am always returning Connections to the head of the queue, they should 
be returned on a get much more often than they appear to be.  I can only assume 
that isDead() is returning TRUE most of the time, so I am discarding most 
Connections that have been returned to the Pool.  My failure occurs when I 
actually reuse a Connection for a subsequent query.

From that evaluation: I have two potential bugs:
  1) Closing Statements in a separate thread is not thread-safe.  (As detailed above.)
  2) Connection.getMetaData() throws SQLExeception for Connections that have been idle for more than a second.

Original issue reported on code.google.com by Brett.Mi...@gmail.com on 24 Oct 2010 at 3:30

[GoogleCodeExporter]

Even if closing a result set / prepared statement / statement is thread save, 
your application would probably still not work as expected. 

> Because Iterator does not provide a close() method

In H2, you don't need to close the result set / prepared statement / statement. 
H2 already does that for you if needed. I know in other databases it is 
required to avoid memory problems, but not in H2.

What about closing the result set when the last row was read (when 
Iterator.hasNext() returns false)?

I will try to make H2 thread-safe so you don't get strange 
NullPointerException, but you will still get an exception (object was closed).

Original comment by thomas.t...@gmail.com on 29 Oct 2010 at 8:17

• Changed state: Accepted

[GoogleCodeExporter]

Fixed in version 1.2.145

You don't get strange NullPointerException, but you probably still get an 
exception (object was closed).

Original comment by thomas.t...@gmail.com on 2 Nov 2010 at 7:29

• Changed state: Fixed