Bug #15570 setReadOnly(...) in ReplicationConnection incorrectly copies state
Submitted: 8 Dec 2005 4:13 Modified: 10 Mar 2006 20:12
Reporter: Jon Siddle Email Updates:
Status: Closed Impact on me:
None 
Category:Connector / J Severity:S2 (Serious)
Version:5.0-nightly-20051208 since at least 3.1 OS:N/A
Assigned to: Mark Matthews CPU Architecture:Any

[8 Dec 2005 4:13] Jon Siddle
Description:
setReadOnly(...) calls switchToMaster() or switchToSlave() regardless of current setting,
and the switchToXXX() methods copy state into the new connection (master/slave) from the OTHER connection rather than the old connection.

How to repeat:
something like

setReadOnly(false);  //change to master
setCatalog("A");      //set master's catalog to "A"
setReadOnly(true);  //change to slave
setCatalog("B");      //set slave's catalog to "B"
setReadOnly(true);  //"change" to slave
//slave's catalog is now incorrectly "A"

Suggested fix:
The following code in ReplicationConnection.java:

...
	public synchronized void setReadOnly(boolean readOnly) throws SQLException {
		if (readOnly) {
			switchToSlavesConnection();
		} else {
			switchToMasterConnection();
		}
	}
...

should be changed to:

...
	public synchronized void setReadOnly(boolean readOnly) throws SQLException {
		if (readOnly) {
			if(currentConnection!=slavesConnection) switchToSlavesConnection();
		} else {
			if(currentConnection!=masterConnection) switchToMasterConnection();
		}
	}
...

(or equivalent).
[8 Dec 2005 11:32] Vasily Kishkin
Could you please provide full text of your test case ? I was not able to reproduce the bug.
[8 Dec 2005 13:28] Aleksey Kishkin
Jon, we made your comment and testcase visible for developers only, as it contatins sensistive info.
[8 Dec 2005 13:32] Jon Siddle
thankyou, I shall re-post with the sensitive info hidden. Sorry for the hassle / duplication!
[8 Dec 2005 13:34] Jon Siddle
Test case attached as TestCase.java
Included below for convenience.

Expected Output:Switching to master
Setting catalog to 'test'
catalog='test'
Switching to slave
catalog='test'
Setting catalog to 'mysql'
catalog='mysql'
Switching to slave
catalog='mysql'

Actual Output:
Switching to master
Setting catalog to 'test'
catalog='test'
Switching to slave
catalog='test'
Setting catalog to 'mysql'
catalog='mysql'
Switching to slave
catalog='test'

(Ie the last line should indicate the new catalog)

Test Case:

import java.sql.*;

public class TestCase {
        public static void main(String args[]) {
                try {
                        Class.forName("com.mysql.jdbc.ReplicationDriver");
                        String url = "jdbc:mysql://192.168.0.1:3308,192.168.0.1:3306/database";
                        Connection conn = DriverManager.getConnection(url, "user", "password");

                        System.err.println("Switching to master");
                        conn.setReadOnly(false);

                        System.err.println("Setting catalog to 'test'");
                        conn.setCatalog("test");
                        System.err.println("catalog='"+conn.getCatalog()+"'");

                        System.err.println("Switching to slave");
                        conn.setReadOnly(true);
                        System.err.println("catalog='"+conn.getCatalog()+"'");

                        System.err.println("Setting catalog to 'mysql'");
                        conn.setCatalog("mysql");
                        System.err.println("catalog='"+conn.getCatalog()+"'");

                        System.err.println("Switching to slave");
                        conn.setReadOnly(true);
                        System.err.println("catalog='"+conn.getCatalog()+"'");

                        conn.close();
                }catch(Exception e){
                        e.printStackTrace();
                }
        }
}
[8 Dec 2005 13:35] Jon Siddle
Simple (if contrived) test case

Attachment: TestCase.java (text/x-java), 989 bytes.

[9 Dec 2005 12:23] Vasily Kishkin
Thanks for the test case. I was able to reproduce the bug. I tested on Windows 2003 and Suse 9.3.
[9 Dec 2005 12:56] Jon Siddle
Proposed fix

Attachment: ReplicationConnection.java.diff (text/x-patch), 436 bytes.

[9 Dec 2005 13:06] Jon Siddle
I've attatched a proposed fix. I've included it below for reference. Having separate methods for switchToMaster and switchToSlave seems a bit redundant anyway, but I'm reluctant to change it since the switchToSlave method has some extra code:

...
	boolean masterAutoCommit = this.masterConnection.getAutoCommit();

		if (this.slavesConnection.getAutoCommit() != masterAutoCommit) {
			this.slavesConnection.setAutoCommit(masterAutoCommit);
		}

		int masterTransactionIsolation = this.masterConnection
				.getTransactionIsolation();

		if (this.slavesConnection.getTransactionIsolation() != masterTransactionIsolation) {
			this.slavesConnection
					.setTransactionIsolation(masterTransactionIsolation);
		}
		this.currentConnection = this.slavesConnection;

*******
		this.slavesConnection.setAutoCommit(this.masterConnection
				.getAutoCommit());
		this.slavesConnection.setTransactionIsolation(this.masterConnection
				.getTransactionIsolation());
*******

The code between *s does not appear in the switchToMaster method. It also appears to be a repetition of the code above it (but programmed in a different style!). I can't see any good reason for this, but if there is one, it should be commented (and the style should be adjusted, or the code factored out).

If this code IS redundant, the switchXXXX methods could be combined into one parameterized function.
I accept that this is a slightly separate issue, but it is related to this bug.

--- src/com/mysql/jdbc/ReplicationConnection.java.orig  2005-12-09 12:44:50.425881032 +0000
+++ src/com/mysql/jdbc/ReplicationConnection.java       2005-12-09 12:44:21.125335392 +0000
@@ -378,6 +378,8 @@
         * @see java.sql.Connection#setReadOnly(boolean)
         */
        public synchronized void setReadOnly(boolean readOnly) throws SQLException {
+               if(readOnly==isReadOnly()) return;
+
                if (readOnly) {
                        switchToSlavesConnection();
                } else {
[13 Dec 2005 10:22] Jon Siddle
Any news guys? Is there anything I can do to get this patch applied more quickly?
If there's a problem with my fix, please let me know.

Cheers
[13 Dec 2005 13:57] Mark Matthews
We're in the middle of qualifying Connector/J 5.0.0 for release, so resources are assigned to that. After 5.0.0 releases later this week, we'll be working through new bugs.

Thanks for the bug report and patch.
[13 Dec 2005 13:59] Jon Siddle
Ah, fair enough. Just wanted to make sure it hadn't slipped through the net.
Good luck with the release!
[24 Feb 2006 22:40] Bugs System
A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/3125
[10 Mar 2006 20:12] Mark Matthews
Fix will be available in 5.0.1 and 3.1.13.
[21 May 2006 13:10] Clive Cox
Looking at the changes in the commit, aren't the swapConnections arguments the wrong way round:

	private synchronized void switchToMasterConnection() throws SQLException {
		swapConnections(this.masterConnection, this.slavesConnection);
	}

	private synchronized void switchToSlavesConnection() throws SQLException {
		swapConnections(this.slavesConnection, this.masterConnection);
	}

given:

private synchronized void swapConnections(Connection sourceConnection, 
			Connection targetConnection) throws SQLException 

So the switch to slave has as target master?
[25 Jun 2009 6:16] Ahm Naum
I was surfing net to find some data related to my project of <a href="http://www.testking.net/testking-642-524.htm">642-524</a> but in the mean time i came here and find this web an interesting one...thx for sharing lot of information,it can be very helpful!
[25 Jun 2009 6:18] Ahm Naum
I was surfing net to find some data related to my project of [url=http://www.testking.net/testking-642-524.htm]642-524[/url] but in the mean time i came here and find this web an interesting one...thx for sharing lot of information,it can be very helpful!