MySQL Bugs: #59462: ConcurrentModificationException inside ConnectionImpl.closeAllOpenStatements()

Bug #59462	ConcurrentModificationException inside ConnectionImpl.closeAllOpenStatements()
Submitted:	13 Jan 2011 7:30	Modified:	21 Apr 2016 17:55
Reporter:	stephane	Email Updates:
Status:	Closed	Impact on me:	None
Category:	Connector / J	Severity:	S2 (Serious)
Version:	5.1.14	OS:	Any
Assigned to:	Alexander Soklakov	CPU Architecture:	Any
Tags:	ConcurrentModificationException

Description:
On a very very fast server we get the following error :

java.util.ConcurrentModificationException
	at java.util.HashMap$HashIterator.nextEntry(HashMap.java:793)
	at java.util.HashMap$KeyIterator.next(HashMap.java:828)
	at com.mysql.jdbc.ConnectionImpl.closeAllOpenStatements(ConnectionImpl.java:1574)
	at com.mysql.jdbc.ConnectionImpl.realClose(ConnectionImpl.java:4454)
	at com.mysql.jdbc.ConnectionImpl.close(ConnectionImpl.java:1555)

This was on driver version 5.1.12, but the same problem also happen with 5.1.14 at line 1583

How to repeat:
The problem can't be systematically reproduced but the cure is known...

Suggested fix:
When looking in the driver method 

The actual driver code is :

	private void closeAllOpenStatements() throws SQLException {
		SQLException postponedException = null;

		if (this.openStatements != null) {
			List currentlyOpenStatements = new ArrayList(); // we need this to avoid ConcurrentModificationEx

			for (Iterator iter = this.openStatements.keySet().iterator(); iter
					.hasNext();) {
				currentlyOpenStatements.add(iter.next());
			}

But should be :

	private void closeAllOpenStatements() throws SQLException {
		SQLException postponedException = null;

		if (this.openStatements != null) {
			List currentlyOpenStatements = new ArrayList(); // we need this to avoid ConcurrentModificationEx

			synchronized(this.openStatements) {
				for (Iterator iter = this.openStatements.keySet().iterator(); iter
						.hasNext();) {
					currentlyOpenStatements.add(iter.next());
				}
			}

Hi Stephane and thanks for a nice catch!

I agree with your assessment of the problem but will consult colleagues before pushing the fix.

Stephane, we have discussed this problem and here's what we think. Although things might be thread-safe that does not necessarily make them shareable across threads. So, are you by any chance using any 3rd party pooling mechanism and/or multi-threading? Note that if you use multi-threading it is your responsibility to keep one thread out of others way in such cases.

We are drawing the conclusion that while one thread is actually in code posted, you are closing the connection from the other thread causing this problem as the only parallelism in the c/J is for canceling statements on timeout.

It would be good if you can describe your code and environment a bit and, if possible, make a test case that occasionally throws this error.

It would also be helpful if we had the full stack trace, so that we can see what's responsible for actually attempting to close the connection out from underneath another thread that's trying to create statements on it.

Basically our application is a server application meant to serve complex industrial catalogs over the web (http://www.compodata.com).
The client part is HTTP/AJAX
The server front-end is PHP
The server back-end is our Java application + MySQL

Facts :
- the application is accessed by requests from PHP (which in turn is accessed by ajax requests via Apache),
- each request use a new (or pooled) thread and the connection to the server is closed at the end of the request,
- there is a notion of session since we are keeping references to lot of objects and states (~100M/session),
- each session keeps database connections opened
- we may request asynchronously for some commands (not every commands since there is a minimal sequence order for most of those commands)
- Our server application may be accessed by customer programs to request specific data information
- The problem happens on a particular server meant for performance, this server is only accessed by customer programs aimed to verify validity and existence of >60000 products description across catalog evolutions, 
- on the customer side there is an oracle db that is filled with validity information 
- this is a particular usage because there is no humans at the other side

The server is a dual-Xeon X5650, IBM SSD drives, 24Go DDR3, 200Mps connexion, Gentoo Linux 64

So yes we are using multi-threading

Hello,

Here is the full exceptions stacks :
java.util.ConcurrentModificationException
	at java.util.HashMap$HashIterator.nextEntry(HashMap.java:793)
	at java.util.HashMap$KeyIterator.next(HashMap.java:828)
	at com.mysql.jdbc.ConnectionImpl.closeAllOpenStatements(ConnectionImpl.java:1574)
	at com.mysql.jdbc.ConnectionImpl.realClose(ConnectionImpl.java:4454)
	at com.mysql.jdbc.ConnectionImpl.close(ConnectionImpl.java:1555)
	at net.lemnik.eodsql.impl.BaseQueryImpl$SingleConnectionSource.close(BaseQueryImpl.java:273)
	at net.lemnik.eodsql.impl.BaseQueryImpl.close(BaseQueryImpl.java:64)
	at net.lemnik.eodsql.impl.BaseQueryImpl$InvokeClose.invoke(BaseQueryImpl.java:179)
	at net.lemnik.eodsql.impl.BaseQueryImpl.invoke(BaseQueryImpl.java:150)
	at $Proxy7.close(Unknown Source)
	at CompoData.sql.DataBase2.CloseDAI(DataBase2.java:307)
	at CompoData.db.Catalog.db2_close(Catalog.java:1901)
	at CompoData.db.Catalog.unLoad(Catalog.java:632)
	at CompoData.db.Catalog.unLoad(Catalog.java:604)
	at CompoData.db.Context.unLoadAll(Context.java:873)
	at CompoData.server.ACEServer.gotoQuit(ACEServer.java:2655)
	at CompoData.server.ACEServer.process_cmd(ACEServer.java:181)
	at CompoData.server.SuperServer$ACEUserConnection.run(SuperServer.java:4042)
	at java.util.concurrent.ThreadPoolExecutor$Worker.runTask(ThreadPoolExecutor.java:886)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:908)
	at java.lang.Thread.run(Thread.java:662)

/

And another one :
java.util.ConcurrentModificationException
	at java.util.HashMap$HashIterator.nextEntry(HashMap.java:793)
	at java.util.HashMap$KeyIterator.next(HashMap.java:828)
	at com.mysql.jdbc.ConnectionImpl.closeAllOpenStatements(ConnectionImpl.java:1574)
	at com.mysql.jdbc.ConnectionImpl.realClose(ConnectionImpl.java:4454)
	at com.mysql.jdbc.ConnectionImpl.close(ConnectionImpl.java:1555)
	at CompoData.sql.DataBase2.delAllConnections(DataBase2.java:107)
	at CompoData.sql.DataBase2.close(DataBase2.java:210)
	at CompoData.db.Catalog.db2_close(Catalog.java:1902)
	at CompoData.db.Catalog.unLoad(Catalog.java:632)
	at CompoData.db.Catalog.unLoad(Catalog.java:604)
	at CompoData.db.Context.unLoadAll(Context.java:873)
	at CompoData.server.ACEServer.gotoQuit(ACEServer.java:2655)
	at CompoData.server.ACEServer.process_cmd(ACEServer.java:181)
	at CompoData.server.SuperServer$ACEUserConnection.run(SuperServer.java:4042)
	at java.util.concurrent.ThreadPoolExecutor$Worker.runTask(ThreadPoolExecutor.java:886)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:908)
	at java.lang.Thread.run(Thread.java:662)

/

Either of the two exceptions happens from time to time, not more than 0.0016% errors

Of course, I could do more synchronization in our code but I don't understand why Connector/J is not reputed to be thread safe... furthermore, there is a already a lot of synchronization work done in Connector/J.

Stephane, thanks for info provided. Before I free up some time to look into traces, I just want to address your last question.

> Of course, I could do more synchronization in our code but I don't understand why Connector/J is not reputed to be thread safe... furthermore, there is a already a lot of synchronization work done in Connector/J.

c/J *IS* thread-safe. So, if we're correct assuming what's happening in your code (see my previous post) then slapping snychronized where you suggested would only lead to NPE in your code as the thread that's calling .close() in parallel will suddenly have nothing to work on! I hope you see the catch... 

It is not good to call close() on object from multiple threads nor there is a good reason for that kind of behavior. Luckily, we are open source so you are free to change the code according to your wishes and see what's happening. In the meantime, we will continue our investigation.

Finally, synchronized is not a magic wand. Especially more so as there is a big stack of apps built on top of driver. Syncing everything would probably break the whole thing apart.

Hello,

To really understand what's really happening here, after more checks, I've to tell you that in my code it's not a double close on connection from two threads, but more a single connection close while another statement is closed during a finalization (so in the finalizer thread) of some other classes. So the exception happens only when I close connection and a GC happens at the same time...

So, to avoid the exception without more synchronization usage, the solution would be to not explicitly close statements, at least inside finalization methods, since closing connection close all yet remaining registered statements inside driver code, but this could be incompatible with other jdbc drivers that do not implicitly close statements at connection close (and just because our app is not theoretically made exclusively for MySQL). Of course, the real solution would be to not forget to immediately close statements everywhere as soon as I don't need those ones... but nobody's perfect!

AFAIK, if it was a double close, I should not get NPE but more a "connection already closed" Exception, and that wouldn't happen since I test if connection is not already closed with isClosed() just before calling close().

Rectification : I would not get an exception at all, close() will just return immediately and silently since its first line code is to return if connection is already closed. So I'll just avoid a call to a synchronized method by calling isClosed() ;-)

Thanks for additional info provided Stephan. If I was really 100% sure this is a case of closing the same connection from two threads, I would have already closed the report which I did not. Still, this is our best bet.

Continuing analysis.

Thanks for further an analysis on that problem,

As I saw that close() was already synchronized on ConnectionImpl instance, I deduced that the problem was obligatorily elsewhere since the double close situation can't happen... 

To my opinion, it could only happen if some thread is doing a StatementImpl.close(), which is synchronized on ConnectionImpl.openStatements object via ConnectionImpl.unregisterStatement() method, while another thread is calling ConnectionImpl.close(), which should be synchronized on ConnectionImpl.openStatements at least while copying.

And I can confirm, that there is a Statement.close() call inside a finalize() method in my code, so the situation may happen from my code.

As a workaround, I may also try dontTrackOpenResources=true but that could lead to potential leaks on the other side

Next week I will try 2 things :
- remove Statement.close() from the finalize()
- add synchronized in the driver

Ok, waiting on your test results.

Hello,

Synchronizing the copy inside ConnectionImpl.closeAllOpenStatements() solved the problem.
Removing Statement.close() in the finalize() in our code also seems to solve or reduce the problem (without modifying Driver).

Stephane, seemingly this Close() in finally block of your code was to blame. By specs, JDBC driver is supposed to close all the connections and you definitely do not need it (as opposed to good practice in some other languages/frameworks).

Adding synchronized in driver code is still not an option due to stack that depends on c/J (various pooling mechanisms, ORM's and so on).

However, I will leave the bug open in case you encounter problem again.

Since we did not hear from Stephane in a month I'm closing the bug.

This is still an issue and happens from time to time in our components as well. Could you please reopen this issue and get it fixed?

Since we have number of examples of 3rd part components where connection can be closed from different maintenance thread I tend to fix this as proposed, by copy sync.

Added the following entry to the MySQL Connector/J 5.1.39 changelog:

"On very fast servers with other third-party components accessing the data, a ConcurrentModificationException was sometimes thrown. This fix prevents the exception by adding a synchronization to ConnectionImpl.closeAllOpenStatements(), and by refactoring part of the code inside the method."

Posted by developer:
 
Added the above-mentioned changelog entry into the Connector/J 6.0.3 changelog, as the fix has been pushed into the 6.0 tree.