Bug #2054 com.mysql.jdbc.Connection.close throws ConcurrentModificationException
Submitted: 9 Dec 2003 4:51 Modified: 9 Dec 2003 8:33
Reporter: [ name withheld ] Email Updates:
Status: Closed Impact on me:
None 
Category:Connector / J Severity:S2 (Serious)
Version:3.1 alpha OS:
Assigned to: Mark Matthews CPU Architecture:Any

[9 Dec 2003 4:51] [ name withheld ]
Description:
java.util.ConcurrentModificationException
	at java.util.HashMap$HashIterator.nextEntry(HashMap.java:782)
	at java.util.HashMap$KeyIterator.next(HashMap.java:818)
	at com.mysql.jdbc.Connection.closeAllOpenStatements(Connection.java:2538)
	at com.mysql.jdbc.Connection.close(Connection.java:1067)

How to repeat:
Do not close used statements.

Suggested fix:
Cause:

Connection.java(2533):
**********************
    private void closeAllOpenStatements() throws SQLException {
        SQLException postponedException = null;

        for (Iterator iter = this.openStatements.keySet().iterator();
                iter.hasNext();) {
            Statement stmt = (Statement) iter.next();

            try {
                stmt.close();
            } catch (SQLException sqlEx) {
                postponedException = sqlEx; // throw it later, cleanup all statements first
            }
        }

        if (postponedException != null) {
            throw postponedException;
        }
    }
**********************
This opens an iterator on openStatements, loop through statements and closes them by calling stmt.close().

Statement.java(790):
**********************
    public synchronized void close() throws java.sql.SQLException {
        if (Driver.TRACE) {
            Object[] args = new Object[0];
            Debug.methodCall(this, "close", args);
        }

        if (results != null) {
            try {
                results.close();
            } catch (Exception ex) {
                ;
            }
        }

        if (this.maxRowsChanged) {
            this.connection.unsetMaxRows(this);
        }

        this.connection.unregisterStatement(this);
        this.results = null;
        this.connection = null;
        this.warningChain = null;
        this.escaper = null;
        this.isClosed = true;
        this.closeAllOpenResults();
        this.openResults = null;
    }
**********************
This closes the statement, and unregister it by calling connection.unregisterStatement().

Connection.java(1926)(again :):
**********************
    synchronized void unregisterStatement(Statement stmt) {
        this.openStatements.remove(stmt);
    }
**********************
Finally, this removes the statement from the openStatements.
So, it modifies a Map while an Iterator is currently iterating through it
in closeAllOpenStatements()!!!

And as Java API specification says for java.util.ConcurrentModificationException:
"...Note that this exception does not always indicate that an object has been concurrently modified by a different thread. If a single thread issues a sequence of method invocations that violates the contract of an object, the object may throw this exception. For example, if a thread modifies a collection directly while it is iterating over the collection with a fail-fast iterator, the iterator will thow this exception..."

Workaround: close all statements after use. :)

Fix suggestion:
Use the iterator's remove() in closeAllOpenStatements().
Cause as Java API specification says for iterator's remove():
"...The behavior of an iterator is unspecified if the underlying collection is modified while the iteration is in progress in any way other than by calling this method..."
This does not affect the unregisterStatement() method.

So:
Connection.java(2533):
**********************
    private void closeAllOpenStatements() throws SQLException {
        SQLException postponedException = null;

        for (Iterator iter = this.openStatements.keySet().iterator();
                iter.hasNext();) {
            Statement stmt = (Statement) iter.next();
            //suggested by gviczai:
            iter.remove();            

            try {
                stmt.close();
            } catch (SQLException sqlEx) {
                postponedException = sqlEx; // throw it later, cleanup all statements first
            }
        }

        if (postponedException != null) {
            throw postponedException;
        }
    }
**********************

I haven't tested this, since I do not have the time but theoratically it works.
:)
[9 Dec 2003 8:33] Mark Matthews
This has already been fixed (in a slightly different way). Please see a nightly snapshot build of Connector/J 3.1 from http://downloads.mysql.com/snapshots.php