* Lock connection during disconnect to prevent some race conditions

* Always use the server version name provided by mysql_get_server_info() as that should me more reliable
* Use mysql_get_server_version() for version comparisons. Less code, official API and closer to what mysql does

4 files changed, 41 insertions, 59 deletions

diff --git a/Frameworks/SPMySQLFramework/Source/SPMySQLConnection Categories/Ping & KeepAlive.m b/Frameworks/SPMySQLFramework/Source/SPMySQLConnection Categories/Ping & KeepAlive.m
index cfbb7183..7940b483 100644
--- a/Frameworks/SPMySQLFramework/Source/SPMySQLConnection Categories/Ping & KeepAlive.m
+++ b/Frameworks/SPMySQLFramework/Source/SPMySQLConnection Categories/Ping & KeepAlive.m
@@ -94,15 +94,15 @@
 		// attempt a single reconnection in the background
 		if (_elapsedSecondsSinceAbsoluteTime(lastConnectionUsedTime) < 60 * 15) {
 			[self _reconnectAfterBackgroundConnectionLoss];
-
+		}
 		// Otherwise set the state to connection lost for automatic reconnect on
 		// next use.
-		} else {
+		else {
 			state = SPMySQLConnectionLostInBackground;
 		}
 
 		// Return as no further ping action required this cycle.
-		goto end;
+		goto end_cleanup;
 	}
 
 	// Otherwise, perform a background ping.
@@ -112,7 +112,7 @@
 	} else {
 		keepAlivePingFailures++;
 	}
-end:
+end_cleanup:
 	keepAliveThread = nil;
 }
 
@@ -139,6 +139,11 @@ end:
 
 	// Set up a query lock
 	[self _lockConnection];
+	//we might find ourselves at the losing end of a contest with -[self _disconnect]
+	if(!mySQLConnection) {
+		[self _unlockConnection];
+		return NO;
+	}
 
 	volatile BOOL keepAliveLastPingSuccess = NO;
 	keepAliveLastPingBlocked = NO;
diff --git a/Frameworks/SPMySQLFramework/Source/SPMySQLConnection Categories/Server Info.m b/Frameworks/SPMySQLFramework/Source/SPMySQLConnection Categories/Server Info.m
index dd684c78..01410eb5 100644
--- a/Frameworks/SPMySQLFramework/Source/SPMySQLConnection Categories/Server Info.m
+++ b/Frameworks/SPMySQLFramework/Source/SPMySQLConnection Categories/Server Info.m
@@ -46,54 +46,34 @@
 		return [NSString stringWithString:serverVariableVersion];
 	}
 
-#warning FIXME: There is probably a race condition here with -[self _disconnect]
-	if(mySQLConnection) {
-		return [self _stringForCString:mysql_get_server_info(mySQLConnection)];
-	}
-	
 	return nil;
 }
 
 /**
- * Return the server major version or NSNotFound on failure
+ * Return the server major version or 0 on failure
  */
 - (NSUInteger)serverMajorVersion
 {
-	NSString *ver;
-	if ((ver = [self serverVersionString]) != nil) {
-		NSString *s = [[ver componentsSeparatedByString:@"."] objectAtIndex:0];
-		return (NSUInteger)[s integerValue];
-	} 
-	
-	return NSNotFound;
+	// 5.5.33 => 50533 / 10'000 => 5.0533 => 5
+	return (serverVersionNumber / 10000);
 }
 
 /**
- * Return the server minor version or NSNotFound on failure
+ * Return the server minor version or 0 on failure
  */
 - (NSUInteger)serverMinorVersion
 {
-	NSString *ver;
-	if ((ver = [self serverVersionString]) != nil) {
-		NSString *s = [[ver componentsSeparatedByString:@"."] objectAtIndex:1];
-		return (NSUInteger)[s integerValue];
-	}
-	
-	return NSNotFound;
+	// 5.5.33 => 50533 - (5*10'000) => 533 / 100 => 5.33 => 5
+	return ((serverVersionNumber - [self serverMajorVersion]*10000) / 100);
 }
 
 /**
- * Return the server release version or NSNotFound on failure
+ * Return the server release version or 0 on failure
  */
 - (NSUInteger)serverReleaseVersion
 {
-	NSString *ver;
-	if ((ver = [self serverVersionString]) != nil) {
-		NSString *s = [[ver componentsSeparatedByString:@"."] objectAtIndex:2];
-		return (NSUInteger)[[[s componentsSeparatedByString:@"-"] objectAtIndex:0] integerValue];
-	}
-	
-	return NSNotFound;
+	// 5.5.33 => 50533 - (5*10'000 + 5*100) => 33
+	return (serverVersionNumber - ([self serverMajorVersion]*10000 + [self serverMinorVersion]*100));
 }
 
 #pragma mark -
@@ -105,23 +85,9 @@
  */
 - (BOOL)serverVersionIsGreaterThanOrEqualTo:(NSUInteger)aMajorVersion minorVersion:(NSUInteger)aMinorVersion releaseVersion:(NSUInteger)aReleaseVersion
 {
-	NSString *ver;
-	if (!(ver = [self serverVersionString])) return NO;
-
-	NSArray *serverVersionParts = [ver componentsSeparatedByString:@"."];
-
-	NSUInteger serverMajorVersion = (NSUInteger)[[serverVersionParts objectAtIndex:0] integerValue];
-	if (serverMajorVersion < aMajorVersion) return NO;
-	if (serverMajorVersion > aMajorVersion) return YES;
-
-	NSUInteger serverMinorVersion = (NSUInteger)[[serverVersionParts objectAtIndex:1] integerValue];
-	if (serverMinorVersion < aMinorVersion) return NO;
-	if (serverMinorVersion > aMinorVersion) return YES;
+	unsigned long myver = aMajorVersion * 10000 + aMinorVersion * 100 + aReleaseVersion;
 
-	NSString *serverReleasePart = [serverVersionParts objectAtIndex:2];
-	NSUInteger serverReleaseVersion = (NSUInteger)[[[serverReleasePart componentsSeparatedByString:@"-"] objectAtIndex:0] integerValue];
-	if (serverReleaseVersion < aReleaseVersion) return NO;
-	return YES;
+	return (myver >= serverVersionNumber);
 }
 
 #pragma mark -
diff --git a/Frameworks/SPMySQLFramework/Source/SPMySQLConnection.h b/Frameworks/SPMySQLFramework/Source/SPMySQLConnection.h
index 444e8dff..6a3a1013 100644
--- a/Frameworks/SPMySQLFramework/Source/SPMySQLConnection.h
+++ b/Frameworks/SPMySQLFramework/Source/SPMySQLConnection.h
@@ -99,6 +99,7 @@
 
 	// Server details
 	NSString *serverVariableVersion;
+	unsigned long serverVersionNumber;
 
 	// Error state for the last query or connection state
 	NSUInteger queryErrorID;
diff --git a/Frameworks/SPMySQLFramework/Source/SPMySQLConnection.m b/Frameworks/SPMySQLFramework/Source/SPMySQLConnection.m
index eb1816b5..1ca2f182 100644
--- a/Frameworks/SPMySQLFramework/Source/SPMySQLConnection.m
+++ b/Frameworks/SPMySQLFramework/Source/SPMySQLConnection.m
@@ -174,6 +174,7 @@ const char *SPMySQLSSLPermissibleCiphers = "DHE-RSA-AES256-SHA:AES256-SHA:DHE-RS
 
 		// Ensure the server detail records are initialised
 		serverVariableVersion = nil;
+		serverVersionNumber = 0;
 
 		// Start with a blank error state
 		queryErrorID = 0;
@@ -475,6 +476,19 @@ asm(".desc ___crashreporter_info__, 0x10");
 	mysqlConnectionThreadId = mySQLConnection->thread_id;
 	lastConnectionUsedTime = initialConnectTime;
 
+	// Copy the server version string to the instance variable
+	if (serverVariableVersion) [serverVariableVersion release], serverVariableVersion = nil;
+	// the mysql_get_server_info() function
+	//   * returns the version name that is part of the initial connection handshake.
+	//   * Unless the connection failed, it will always return a non-null buffer containing at least a '\0'.
+	//   * It will never affect the error variables (since it only returns a struct member)
+	//
+	// At that point (handshake) there is no charset and it's highly unlikely this will ever contain something other than ASCII,
+	// but to be safe, we'll use the Latin1 encoding which won't bail on invalid chars.
+	serverVariableVersion = [[NSString alloc] initWithCString:mysql_get_server_info(mySQLConnection) encoding:NSISOLatin1StringEncoding];
+	// this one can actually change the error state, but only if the server version string is not set (ie. no connection)
+	serverVersionNumber = mysql_get_server_version(mySQLConnection);
+
 	// Update SSL state
 	connectedWithSSL = NO;
 	if (useSSL) connectedWithSSL = (mysql_get_ssl_cipher(mySQLConnection))?YES:NO;
@@ -892,6 +906,7 @@ asm(".desc ___crashreporter_info__, 0x10");
 	[self _unlockConnection];
 	[self _cancelKeepAlives];
 
+	[self _lockConnection];
 	// Close the underlying MySQL connection if it still appears to be active, and not reading
 	// or writing.  While this may result in a leak of the MySQL object, it prevents crashes
 	// due to attempts to close a blocked/stuck connection.
@@ -899,17 +914,16 @@ asm(".desc ___crashreporter_info__, 0x10");
 		mysql_close(mySQLConnection);
 	}
 	mySQLConnection = NULL;
+	if (serverVariableVersion) [serverVariableVersion release], serverVariableVersion = nil;
+	serverVersionNumber = 0;
+	if (database) [database release], database = nil;
+	state = SPMySQLDisconnected;
+	[self _unlockConnection];
 
 	// If using a connection proxy, disconnect that too
 	if (proxy) {
 		[proxy performSelectorOnMainThread:@selector(disconnect) withObject:nil waitUntilDone:YES];
 	}
-
-	// Clear host-specific information
-	if (serverVariableVersion) [serverVariableVersion release], serverVariableVersion = nil;
-	if (database) [database release], database = nil;
-
-	state = SPMySQLDisconnected;
 }
 
 /**
@@ -934,10 +948,6 @@ asm(".desc ___crashreporter_info__, 0x10");
 		[variables setObject:[variableRow objectAtIndex:1] forKey:[variableRow objectAtIndex:0]];
 	}
 
-	// Copy the server version string to the instance variable
-	if (serverVariableVersion) [serverVariableVersion release], serverVariableVersion = nil;
-	serverVariableVersion = [[variables objectForKey:@"version"] retain];
-
 	// Get the connection encoding.  Although a specific encoding may have been requested on
 	// connection, it may be overridden by init_connect commands or connection state changes.
 	// Default to latin1 for older server versions.