Обсуждение: 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

On Wed, 20 Dec 2006, Morten Andersen wrote:

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

Is this something you can reproduce somewhat regularly or something that
just happened once?

> 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
>
> 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.
>

Neither of these look particularly likely to me.  All entry points to
QueryExecutorImpl that access these items are synchronized so I don't
think the thread safety of the individual items are an issue.

Is your application multi-threaded and using a Connection per thread or
does it use multiple threads per connection?  If using Connection per
thread it's even harder to believe this is a thread safety issue because
you'll have a QueryExecutorImpl per Connection.

Kris Jurka

Hi Kris,

Kris Jurka wrote:
>
>> In a multithreaded application I just got an unexpected
>> NullPointerException from the depth's of the JDBC-drivers cleanup
>> routine of parsed queries.
>>
>
> Is this something you can reproduce somewhat regularly or something that
> just happened once?

Unfortunately it is not reproduceable :-) - I just saw it once, in a
long running application with heavy load on the driver.

[SNIP]
>
> Neither of these look particularly likely to me.  All entry points to
> QueryExecutorImpl that access these items are synchronized so I don't
> think the thread safety of the individual items are an issue.

After re-inspection, I think you are right. I definitely had a too
limited "scope" when looking for the cause, since I mostly/only looked
at registerOpenPortal() and processDeadPortals().

>
> Is your application multi-threaded and using a Connection per thread or
> does it use multiple threads per connection?  If using Connection per
> thread it's even harder to believe this is a thread safety issue because
> you'll have a QueryExecutorImpl per Connection.

Multi-threaded, with only one thread using one connection at a time, but
using an Apache Commons Pool, so over time the same connection will be
used from several threads.

But given the issues with the synchronization on all entry points, as
you pointed out, I think you are right, in that neither of my two
suggestions to what the cause of the original stacktrace could be, are
correct.

So since I don't have any other good ideas to what it could be, and
since I only saw it once (and nobody else reported a similar issue), we
should probably just ignore the error as not-reproducable.

But thanks for taking your time to answer, and for looking into this issue.

Best Regards
Morten Andersen,
Instadia A/S, Denmark