Bug 53995 - AbstractJDBCTestElement shares PreparedStatement between multi-threads
AbstractJDBCTestElement shares PreparedStatement between multi-threads
Status: RESOLVED FIXED
Product: JMeter
Classification: Unclassified
Component: Main
2.7
All All
: P2 critical (vote)
: ---
Assigned To: JMeter issues mailing list
: PatchAvailable
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2012-10-12 07:56 UTC by Haixing Hu
Modified: 2012-10-27 14:53 UTC (History)
2 users (show)



Attachments
The patch for JMeter 2.7-rc3, fix the shared PreparedStatement bug. (6.33 KB, patch)
2012-10-12 07:56 UTC, Haixing Hu
Details | Diff
Test Plan showing issue (6.41 KB, application/xml)
2012-10-14 16:38 UTC, Philippe Mouawad
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Haixing Hu 2012-10-12 07:56:30 UTC
Created attachment 29468 [details]
The patch for JMeter 2.7-rc3, fix the shared PreparedStatement bug.

The implementation of AbstractJDBCTestElement uses a map to cache the PreparedStatement for each Connection, and shares that cache between multi-threads, which is the cause of the bug.

According to the section 13.1.4 of JDBC 4.0 specification (http://jcp.org/aboutJava/communityprocess/final/jsr221/index.html), 

> 13.1.4 Closing Statement Objects
> An application calls the method Statement.close to indicate that it 
> has finished processing a statement. All Statement objects will be closed
> when the con- nection that created them is closed. However, it is good coding
> practice for applications to close statements as soon as they have finished 
> processing them. This allows any external resources that the statement is 
> using to be released immediately.
> Closing a Statement object will close and invalidate any instances of 
> ResultSet produced by that Statement object. The resources held by the
> ResultSet object may not be released until garbage collection runs again, 
> so it is a good practice to explicitly close ResultSet objects when they 
> are no longer needed.
> Once a Statement has been closed, any attempt to access any of its methods 
> with the exception of the isClosed or close methods will result in a
> SQLException being thrown.
> These comments about closing Statement objects apply to PreparedStatement 
> and CallableStatement objects as well.

It's clear that closing a PreparedStatement object will automatically close all the ResultSet objects opened by it. Therefore, if a PreparedStatement is shared between thread A and thread B, and the thread A is closing the PreparedStatement object while thread B is still in use of a ResultSet object opened by that PreparedStatement object, the ResultSet object will be closed indirectly by thread A without notifying thread B. This situation could happen, since the cache of PreparedStatement has a capacity limit and it will automatically close an old cached PreparedStatement object while adding a new PreparedStatement to the cache reaching the capacity limit.

It's also easy to reproduce the bug: create a JDBC testing plan using a prepread statement, with 300 threads and 2000 loops; then run the test, the bug *may* occur. If the bug does not occur, try to increase the number of threads and the number of loops. The log of the JDBC driver shows that it is caused by using a closed ResultSet object.

The attachment is the patch for Apache JMeter v2.7-rc3.

The patch does the following works:

1. change the cache of PreparedStatement in AbstractJDBCTestElement to a thread local cache;
2. add some logging statements in order to help locating the bug;
3. fix the location of "jcharts" component in the "build.properties" configuration file, since the "www.mvnsearch.org" was moved.
Comment 1 Philippe Mouawad 2012-10-14 16:22:36 UTC
I am not sure your analysis is correct.
There are 2 modes for JDBC Connection Configuration.

If max pool is set to 0, there will be one pool for each thread, connections will never be shared, so this one is OK

Now, if max pool is set, it can happen at one time that 2 threads will use the same connection, but in my understanding, it will never happen at the same time as pool will return an idle connection (so different from the one in use) or it will wait if no one is available.

So the case you describe cannot happen.

It is true that the pool shares PreparedStatement but they are always associated to the same connection. So I don't think there is an issue as connection will never be shared by two threads at the same time, but I may be wrong.

Could you submit a Test Plan showing the issue and the exact error ? or one of the 2 issues ?

Thank you.
Comment 2 Philippe Mouawad 2012-10-14 16:28:34 UTC
Ok I saw issue, the issue is that one thread A can reach limit of perConnCache generating a closeAllStatements() which can impact thread B using this cache.
Comment 3 Philippe Mouawad 2012-10-14 16:38:26 UTC
Created attachment 29476 [details]
Test Plan showing issue

To provoke bug, set:
jdbcsampler.cachesize=1
Comment 4 Philippe Mouawad 2012-10-27 14:53:09 UTC
Thank you for the reporting, the analysis and the patch.
The proposed patch has the impact of creating one Cache per Thread and connection which increases memory usage during test.

I implemented another fix which removes the MRU cache as it is the source of issue, since the put of a connection or PreparedStatement Map could result into the putter thread provoking a cleanup (and close) of PreparedStatement of other thread.

This should not impact very much memory as size of first map is the size of number of threads.
Size of second map is the size of different SQL queries of the test.
Comment 5 Philippe Mouawad 2012-10-27 14:53:45 UTC
Date: Sat Oct 27 14:50:28 2012
New Revision: 1402802

URL: http://svn.apache.org/viewvc?rev=1402802&view=rev
Log:
Bug 53995 - AbstractJDBCTestElement shares PreparedStatement between multi-threads
Bugzilla Id: 53995

Modified:
    jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/AbstractJDBCTestElement.java
    jmeter/trunk/xdocs/changes.xml