Unexpected NullPointerException in "processDeadParsedQueries()" - suggested fix

Hi,

In a multithreaded application I just got an unexpected
NullPointerException from the depth's of the JDBC-drivers cleanup
routine of parsed queries.

The driver I am using is the most recent JDBC3 driver, version 8.2-504.

During a batch-execution of a trivial PreparedStatement that generally
works fine I got the NullPointerException below:

Exception in thread "Thread-5" java.lang.NullPointerException
at org.postgresql.core.Utils.encodeUTF8(Utils.java:54)
at
org.postgresql.core.v3.QueryExecutorImpl.sendCloseStatement(QueryExecutorImpl.java:982)
at
org.postgresql.core.v3.QueryExecutorImpl.processDeadParsedQueries(QueryExecutorImpl.java:1104)
at
org.postgresql.core.v3.QueryExecutorImpl.sendQueryPreamble(QueryExecutorImpl.java:364)
at
org.postgresql.core.v3.QueryExecutorImpl.execute(QueryExecutorImpl.java:322)
at
org.postgresql.jdbc2.AbstractJdbc2Statement.executeBatch(AbstractJdbc2Statement.java:2592)

[SNIP - rest of calling stacktrace - was outside the driver]

I have been browsing the CVS-repository and I think the code that
generates this is the processDeadParsedQueries() below:

private void processDeadParsedQueries() throws IOException {
PhantomReference deadQuery;
while ((deadQuery = (PhantomReference)parsedQueryCleanupQueue.poll())
!= null)
{
String statementName = (String)parsedQueryMap.remove(deadQuery);
<--- RETURNS null?
sendCloseStatement(statementName); <--- TRIGGERS NullPointerException
deadQuery.clear();
}
}

I inserted the comments with "<---".

The reaon why it is possible for the parsedQueryMap to return a null
statementName for the deadQuery key is quite a puzzle to me. Mostly I
can only think of two reasons:

1) The statementName for the deadQuery has already been removed. Given
the code for registerParsedQuery() and processDeadParsedQueries() I
can't see how this should be possible, as the
java.lang.ref.ReferenceQueue is thread-safe (it doesn't say so in the
API documentation, but that is what you would expect, and a quick glance
at the JDK source also indicates that this is the case).

2) The use of HashMap for the parsedQueryMap could be a problem in a
multithreaded application, since it is not thread-safe? So a wild guess
is that the thread executing the processDeadParsedQueries() does not
"see" the value that was added to the hashmap by another thread in the
registerParsedQuery.

Maybe some of you guys can see other ways this situation could arise?
Otherwise I would suggest a combination of two solutions:

a) change parsedQueryMap to a synchronizedMap (either by the
Collections.synchronizedMap or by using a Hashtable - or for JDK1.5+ the
more "highspeed" java.util.concurrent.ConcurrentHashMap).

and

b) do an explicit check that the returned statementName is not null,
i.e. change the body of the while-loop in processDeadParsedQueries to
something along the lines:

String statementName = (String)parsedQueryMap.remove(deadQuery);
if (statementName != null) {
sendCloseStatement(statementName);
}
deadQuery.clear();

This should not be necessary (as far as I can tell?), since the
ReferenceQueue should never contain the deadQuery before it has been
added to the parsedQueryMap in registerParsedQuery() due to the contract
for PhantomReference? But one should always be carefull with this kind
of Thread-reasoning :-)

Please feel to come with any thoughts on this fix, and if you need more
information on the exact code that executed the PreparedStatement,
please also mail me.

Best Regards
Morten Andersen,
Instadia A/S, Denmark