Bug #61247 Deadlock in case error occurred during StatementImpl#executeQuery call
Submitted: 20 May 2011 16:59 Modified: 4 Apr 2013 11:27
Reporter: Amigo V. Email Updates:
Status: Closed Impact on me:
None 
Category:Connector / J Severity:S2 (Serious)
Version:5.1.16 OS:Any
Assigned to: Alexander Soklakov CPU Architecture:Any
Tags: jdbc deadlock executeQuery, regression

[20 May 2011 16:59] Amigo V.
Description:
If IO error occurred during executing query (and property dontTrackOpenResources is set to false, it's it default value) then it'll try to close all open statements (method ConnectionImpl#closeAllOpenStatements) and this may cause deadlock if any execute*(*) method of other instance of StatementImpl class is called by other thread.
See example.

How to repeat:
public class MysqlDriverDeadlockTest {
    public static void main(String[] args) throws Exception {
        Class.forName("com.mysql.jdbc.Driver").newInstance();

        Properties connInfo = new Properties();

        connInfo.put("user", "root");
        connInfo.put("password", "new-password");
        ConnectionImpl conn = (ConnectionImpl) DriverManager.getConnection("jdbc:mysql://titan/game", connInfo);

        TestThread threadA = new TestThread("Thread A", conn.createStatement());
        threadA.start();

        TestThread threadB = new TestThread("Thread B", conn.createStatement());
        threadB.start();

        //lazy error imitation 
        Field io = ConnectionImpl.class.getDeclaredField("io");
        io.setAccessible(true);
        Method forceClose = MysqlIO.class.getDeclaredMethod("forceClose");
        forceClose.setAccessible(true);

        forceClose.invoke(io.get(conn));

        Thread.sleep(1000);

        ThreadMXBean tmbean = ManagementFactory.getThreadMXBean();
        if (tmbean.findDeadlockedThreads().length > 0) {
            System.out.println("deadlock!");
        }
    }
}

class TestThread extends Thread {
    private final Statement statement;

    TestThread(String name, Statement statement) {
        super(name);
        this.statement = statement;
    }

    public void run() {
        try {
            statement.executeQuery("SELECT SLEEP (30)");
        } catch (SQLException e) {
            e.printStackTrace();
        }
    }
}

Suggested fix:
remove synchronized from execute*(*) methods, there is already synchronization on the class field connection;
[20 May 2011 17:08] Mark Matthews
Is there a reason you *must* share a connection between threads? Even through the JDBC specification requires a driver implementation to be thread-safe, it doesn't guarantee that it won't deadlock (those are two separate concerns). Because transactions are scoped to connections, and there are various requirements built into JDBC (closing all open statements on connection close, closing open result sets on Statement.execute*(), etc), this is always going to be deadlock-prone.
[20 May 2011 17:29] Amigo V.
well, it's old application, and it works for years without such problems
and it works with driver 5.1.13, coz there is no double synchronization, see this:
    public synchronized java.sql.ResultSet executeQuery(String sql) throws SQLException {
        checkClosed();

        MySQLConnection locallyScopedConn = this.connection;

        synchronized (locallyScopedConn) {

Though without this additional synchronization the "synchronized (locallyScopedConn) {" may cause NPE, but imho NPE is better than deadlock :), because application may continue working and restore connection.
[20 May 2011 17:34] Mark Matthews
The synchronization changes added in 5.1.16 were for correctness measured against essentially everything into a strictly serializable synchronization model, your application is relying on (arguably, I guess?) buggy behavior.

Since technically, almost all databases will require one statement to complete before another can begin, and JDBC itself doesn't provide any guarantees about how this is enforced, for correctness sake, multi-threaded applications sharing a connection, should synchronize on said connection, or some other common mutex.

This bug has not yet been triaged, but realize it is not a common use case to use one connection from multiple threads, which will probably affect the priority of fixing this issue.
[20 May 2011 19:37] Amigo V.
I see, but it was safe to 'share' connection: the synchronization on connection inside driver (inside method execute*() the “synchronized (locallyScopedConn) {“) doesn't allow you to really 'share' the connection and it's transparent for application. 

But now I should make additional synchronization on the same connection (of course I can use other mutex) outside of this method. Imho it’s not obvious and ‘wrong’…
[28 Jun 2011 15:03] Tonci Grgin
There's a valid bug here, but it requires quite some work and testing on different frameworks to be fixed properly. Cleaning this up entirely would mean that connection usage is essentially serialized, which fails with other frameworks. We may be able to clean up this error condition to avoid the deadlock, but proper fix should not happen in 5.1 branch.
[4 Mar 2013 10:31] Alexander Soklakov
Hi Amigo,

This bug was fixed in 5.1.21 by overall synchronization strategy change. We'll close this report but feel free to reopen it if you find the problem in latest versions.
[4 Apr 2013 11:27] Alexander Soklakov
Added to changelog for 5.1.21:

By default, if an I/O error occurred while executing a query a
deadlock could occur as Connector/J attempted to close all open
statements (by calling method ConnectionImpl#closeAllOpenStatements)
The deadlock would occur if any execute*(*) method of another
instance of the StatementImpl class was called by another thread. A
workaround was to set the property dontTrackOpenResources to true.