Bug 60645

Summary: StatementFinalizer is not thread-safe
Product: Tomcat Modules Reporter: baier
Component: jdbc-poolAssignee: Tomcat Developers Mailing List <dev>
Status: NEW ---    
Severity: normal Keywords: PatchAvailable
Priority: P2    
Version: unspecified   
Target Milestone: ---   
Hardware: PC   
OS: All   
Attachments: A thread-safe variant of the original StatementFinalizer

Description baier 2017-01-25 16:05:12 UTC
The interceptor

org.apache.tomcat.jdbc.pool.interceptor.StatementFinalizer

is not thread-safe. 

Some JDBC drivers (e.g. MSSQL) allow to use a single JDBC connection in multiple threads (a statement cannot be shared across threads). 

In our application we use multiple different threads which operate on the same JDBC connection. If a statement is created then the StatementFinalizer stores it in a java.util.LinkedList (which is not thread-safe and may damage the data structure).

This might lead to a

java.lang.NullPointerException
	at java.util.LinkedList.unlink(LinkedList.java:211)
	at java.util.LinkedList.remove(LinkedList.java:526)
	at org.apache.tomcat.jdbc.pool.interceptor.StatementFinalizer.closeInvoked(StatementFinalizer.java:59)
	at org.apache.tomcat.jdbc.pool.interceptor.AbstractCreateStatementInterceptor.invoke(AbstractCreateStatementInterceptor.java:59)
	at org.apache.tomcat.jdbc.pool.JdbcInterceptor.invoke(JdbcInterceptor.java:108)
	at org.apache.tomcat.jdbc.pool.interceptor.ConnectionState.invoke(ConnectionState.java:152)
	at org.apache.tomcat.jdbc.pool.JdbcInterceptor.invoke(JdbcInterceptor.java:108)
	at org.apache.tomcat.jdbc.pool.TrapException.invoke(TrapException.java:40)


Our workaround was to create our own thread-safe statement finalizer.
Comment 1 Christopher Schultz 2017-01-25 20:03:52 UTC
The JDBC spec doesn't mandate thread-safety for the various classes, so you shouldn't expect them to be thread-safe.

That said, it seems like StatementFinalizer should probably be thread-safe in case the JDBC driver itself guarantees thread safety. Or, even better, a MT-safe StatementFinalizer could be provided when it is required (which I would think would be an unusual situation).

Would you care to donate your MT-safe StatementFinalizer class to the project?
Comment 2 baier 2017-01-26 08:59:20 UTC
Created attachment 34679 [details]
A thread-safe variant of the original StatementFinalizer

This is a slightly modified version of the original StatementFinalizer. There are only two changes:

1) It uses an ArrayList instead of a LinkedList.
2) The access on the "statements" list is synchronized.

Note that I don't think a more sophisticated implementation (e.g. with a concurrent copy-on-write datastructure) is needed because I'd not expect a high thread contention.
Comment 3 Christopher Schultz 2017-01-27 15:23:53 UTC
Could this class have been written as a subclass of StatementFinalizer that simply overrides all the methods with synchronized versions and delegates to the superclass? That would be less code to maintain.
Comment 4 baier 2017-01-27 16:17:03 UTC
(In reply to Christopher Schultz from comment #3)
> Could this class have been written as a subclass of StatementFinalizer that
> simply overrides all the methods with synchronized versions and delegates to
> the superclass? That would be less code to maintain.

The new StatementFinalizer class also replaces the LinkedList with an ArrayList (but this is not the important change - I think it would also be okay if you would keep the original LinkedList implementation).

But why would you want to keep both the original StatementFinalizer class and the thread-safe StatementFinalizer class? I think it would be really good to have just one StatementFinalizer class which is thread-safe.
Comment 5 Christopher Schultz 2017-01-27 18:52:40 UTC
(In reply to baier from comment #4)
> But why would you want to keep both the original StatementFinalizer class
> and the thread-safe StatementFinalizer class?

Synchronization is not necessary for most users, so they don't need to pay the penalty. Yes, uncontested locks are fairly inexpensive, but when they aren't needed at all, they can be completely eliminated.