aboutsummaryrefslogtreecommitdiffstats
path: root/Frameworks/MCPKit
diff options
context:
space:
mode:
authorjakob <jakob@eggerapps.at>2010-04-16 17:18:54 +0000
committerjakob <jakob@eggerapps.at>2010-04-16 17:18:54 +0000
commitf5f15a660b5663865347e2b2b162fba7ad86566f (patch)
tree5c8f1093381e02563f4b816c96a6848bf9efac3d /Frameworks/MCPKit
parent4c7dbcd724e9ac6fbd5943f147b15472833dceeb (diff)
downloadsequelpro-f5f15a660b5663865347e2b2b162fba7ad86566f.tar.gz
sequelpro-f5f15a660b5663865347e2b2b162fba7ad86566f.tar.bz2
sequelpro-f5f15a660b5663865347e2b2b162fba7ad86566f.zip
- changed the query locking mechanism for MCPConnection to be more thread safe. From now on, always use [self lockConnection] rather than [queryLock lock], independent of what thread you are running on
- A warning is written to the console when the connection is unlocked multiple times (to identify potential race conditions) - modified MCPStreamingResult to ensure it only closes the connection once - added a check to prevent arrow key navigation past the last row
Diffstat (limited to 'Frameworks/MCPKit')
-rw-r--r--Frameworks/MCPKit/MCPFoundationKit/MCPConnection.h7
-rw-r--r--Frameworks/MCPKit/MCPFoundationKit/MCPConnection.m138
-rw-r--r--Frameworks/MCPKit/MCPFoundationKit/MCPConstants.h7
-rw-r--r--Frameworks/MCPKit/MCPFoundationKit/MCPStreamingResult.m44
4 files changed, 127 insertions, 69 deletions
diff --git a/Frameworks/MCPKit/MCPFoundationKit/MCPConnection.h b/Frameworks/MCPKit/MCPFoundationKit/MCPConnection.h
index dd8a172e..10d46df2 100644
--- a/Frameworks/MCPKit/MCPFoundationKit/MCPConnection.h
+++ b/Frameworks/MCPKit/MCPFoundationKit/MCPConnection.h
@@ -134,8 +134,10 @@
NSUInteger mConnectionFlags; /* The flags to be used for the connection to the database. */
id delegate; /* Connection delegate */
-
- NSLock *queryLock; /* Anything that performs a mysql_net_read is not thread-safe: mysql queries, pings */
+
+ /* Anything that performs a mysql_net_read is not thread-safe: mysql queries, pings */
+ /* Always lock the connection first. Don't use this lock directly, use the lockConnection method! */
+ NSConditionLock *connectionLock;
BOOL useKeepAlive;
BOOL isDisconnecting;
@@ -278,6 +280,7 @@ void performThreadedKeepAlive(void *ptr);
// Locking
- (void)lockConnection;
+- (BOOL)tryLockConnection;
- (void)unlockConnection;
// Database structure
diff --git a/Frameworks/MCPKit/MCPFoundationKit/MCPConnection.m b/Frameworks/MCPKit/MCPFoundationKit/MCPConnection.m
index ad2f72e4..a94584ee 100644
--- a/Frameworks/MCPKit/MCPFoundationKit/MCPConnection.m
+++ b/Frameworks/MCPKit/MCPFoundationKit/MCPConnection.m
@@ -90,9 +90,12 @@ static BOOL sTruncateLongFieldInLogs = YES;
mEncoding = NSISOLatin1StringEncoding;
mConnectionFlags = kMCPConnectionDefaultOption;
-
- queryLock = [[NSLock alloc] init];
-
+
+ // Anything that performs a mysql_net_read is not thread-safe: mysql queries, pings
+ // Always lock the connection first. Don't use this lock directly, use the lockConnection method!
+ connectionLock = [[NSConditionLock alloc] initWithCondition:MCPConnectionIdle];
+ [connectionLock setName:@"MCPConnection connectionLock"];
+
connectionHost = nil;
connectionLogin = nil;
connectionSocket = nil;
@@ -642,7 +645,7 @@ static BOOL sTruncateLongFieldInLogs = YES;
- (BOOL)pingConnection
{
// Set up a query lock
- [queryLock lock];
+ [self lockConnection];
uint64_t currentTime_t;
Nanoseconds elapsedTime;
@@ -671,7 +674,7 @@ static BOOL sTruncateLongFieldInLogs = YES;
}
pthread_attr_destroy(&attr);
- [queryLock unlock];
+ [self unlockConnection];
return lastPingSuccess;
}
@@ -708,10 +711,11 @@ void pingConnectionTask(void *ptr)
return;
}
- // Attempt to get a query lock, but release it to ensure the connection isn't locked
- // by a background ping.
- if (![queryLock tryLock]) return;
- [queryLock unlock];
+ // Attempt to lock the connection. If the connection currently is busy,
+ // we don't need a ping. The connection is unlocked in threadedKeepAlive
+ // before the ping actually returns, to prevent the ping from delaying
+ // other queries
+ if (![self tryLockConnection]) return;
// Store the ping time
lastKeepAliveTime = timeConnected;
@@ -726,7 +730,11 @@ void pingConnectionTask(void *ptr)
*/
- (void)threadedKeepAlive
{
- if (!mConnected || keepAliveThread != NULL) return;
+ if (!mConnected || keepAliveThread != NULL) {
+ // unlock the connection now. it has been locked in keepAlive:
+ [self unlockConnection];
+ return;
+ }
// Use a ping timeout between zero and thirty seconds
NSInteger pingTimeout = 30;
@@ -738,6 +746,9 @@ void pingConnectionTask(void *ptr)
pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);
pthread_create(&keepAliveThread, &attr, (void *)&performThreadedKeepAlive, (void *)mConnection);
+ // unlock the connection now. it has been locked in keepAlive:
+ [self unlockConnection];
+
// Give the connection time to respond, but force a timeout after the ping timeout
// if the thread hasn't already closed itself.
sleep(pingTimeout);
@@ -1129,13 +1140,12 @@ void performThreadedKeepAlive(void *ptr)
if (mConnected) {
const char *theDBName = [self cStringFromString:dbName];
- [queryLock lock];
+ [self lockConnection];
if (0 == mysql_select_db(mConnection, theDBName)) {
- [queryLock unlock];
-
+ [self unlockConnection];
return YES;
}
- [queryLock unlock];
+ [self unlockConnection];
}
[self setLastErrorMessage:nil];
@@ -1444,10 +1454,7 @@ void performThreadedKeepAlive(void *ptr)
}
}
- // Lock the connection - on this thread for normal result sets (avoiding blocking issues
- // when the app is in modal mode), or ensuring a lock on the main thread for streaming queries.
- if (streamResultType == MCPStreamingNone) [queryLock lock];
- else [self lockConnection];
+ [self lockConnection];
// Run (or re-run) the query, timing the execution time of the query - note
// that this time will include network lag.
@@ -1466,7 +1473,7 @@ void performThreadedKeepAlive(void *ptr)
// For normal result sets, fetch the results and unlock the connection
if (streamResultType == MCPStreamingNone) {
theResult = [[MCPResult alloc] initWithMySQLPtr:mConnection encoding:mEncoding timeZone:mTimeZone];
- if (!queryCancelled || !queryCancelUsedReconnect) [queryLock unlock];
+ if (!queryCancelled || !queryCancelUsedReconnect) [self unlockConnection];
// For streaming result sets, fetch the result pointer and leave the connection locked
} else if (streamResultType == MCPStreamingFast) {
@@ -1482,8 +1489,7 @@ void performThreadedKeepAlive(void *ptr)
break;
}
} else {
- if (streamResultType == MCPStreamingNone) [queryLock unlock];
- else [self unlockConnection];
+ [self unlockConnection];
}
queryErrorMessage = [[NSString alloc] initWithString:@""];
@@ -1495,8 +1501,7 @@ void performThreadedKeepAlive(void *ptr)
// On failure, set the error messages and IDs
} else {
if (!queryCancelled || !queryCancelUsedReconnect) {
- if (streamResultType == MCPStreamingNone) [queryLock unlock];
- else [self unlockConnection];
+ [self unlockConnection];
}
if (queryCancelled) {
@@ -1586,8 +1591,8 @@ void performThreadedKeepAlive(void *ptr)
if (![self isConnected]) return;
// Check whether a query is actually being performed - if not, also return.
- if ([queryLock tryLock]) {
- [queryLock unlock];
+ if ([self tryLockConnection]) {
+ [self unlockConnection];
return;
}
@@ -1677,32 +1682,55 @@ void performThreadedKeepAlive(void *ptr)
#pragma mark Connection locking
/**
- * Lock the connection from any thread; ensure the the connection is locked on
- * the main thread, but as fast as possible.
+ * Lock the connection. This must be done before performing any operation
+ * that is not thread safe, eg. performing queries or pinging.
*/
- (void)lockConnection
{
- if ([NSThread isMainThread]) [queryLock lock];
- else [queryLock performSelectorOnMainThread:@selector(lock) withObject:nil waitUntilDone:YES];
+ // We can only start a query as soon as the condition is MCPConnectionIdle
+ [connectionLock lockWhenCondition:MCPConnectionIdle];
+
+ // We now set the condition to MCPConnectionBusy
+ [connectionLock unlockWithCondition:MCPConnectionBusy];
}
/**
- * Unlock the connection from any thread; ensure the connection is unlocked on
- * the main thread, but as fast as possible.
+ * Try locking the connection. If the connection is idle (unlocked), this method
+ * locks the connection and returns YES. The connection must afterwards be unlocked
+ * using unlockConnection. If the connection is currently busy (locked), this
+ * method immediately returns NO and doesn't lock the connection.
*/
-- (void)unlockConnection
+- (BOOL)tryLockConnection
{
+ // check if the condition is MCPConnectionIdle
+ if ([connectionLock tryLockWhenCondition:MCPConnectionIdle]) {
+ // We're allowed to use the connection!
+ [connectionLock unlockWithCondition:MCPConnectionBusy];
+ return YES;
+ } else {
+ // Someone else is using the connection right now
+ return NO;
+ }
+}
- // Ensure the unlock occurs on the main thread
- if (![NSThread isMainThread]) {
- [self performSelectorOnMainThread:@selector(unlockConnection) withObject:nil waitUntilDone:NO];
- return;
- }
- // Unlock the connection, first ensuring it is locked to avoid
- // multiple unlock call issues (eg reconnected queries, threading)
- [queryLock tryLock];
- [queryLock unlock];
+/**
+ * Unlock the connection.
+ */
+- (void)unlockConnection
+{
+ // We don't care if the connection is busy or not
+ [connectionLock lock];
+
+ // We check if the connection actually was busy. If it wasn't busy,
+ // it means we probably tried to unlock the connection twice. This is
+ // potentially dangerous, therefore we log this to the console
+ if ([connectionLock condition]!=MCPConnectionBusy) {
+ NSLog(@"Tried to unlock the connection, but it wasn't locked.");
+ }
+
+ // We tell everyone that the connection is available again!
+ [connectionLock unlockWithCondition:MCPConnectionIdle];
}
#pragma mark -
@@ -1731,7 +1759,7 @@ void performThreadedKeepAlive(void *ptr)
if (![self checkConnection]) return [[[MCPResult alloc] init] autorelease];
- [queryLock lock];
+ [self lockConnection];
if ((dbsName == nil) || ([dbsName isEqualToString:@""])) {
if (theResPtr = mysql_list_dbs(mConnection, NULL)) {
theResult = [[MCPResult alloc] initWithResPtr: theResPtr encoding: mEncoding timeZone:mTimeZone];
@@ -1750,7 +1778,7 @@ void performThreadedKeepAlive(void *ptr)
theResult = [[MCPResult alloc] init];
}
}
- [queryLock unlock];
+ [self unlockConnection];
if (theResult) {
[theResult autorelease];
@@ -1784,7 +1812,7 @@ void performThreadedKeepAlive(void *ptr)
if (![self checkConnection]) return [[[MCPResult alloc] init] autorelease];
- [queryLock lock];
+ [self lockConnection];
if ((tablesName == nil) || ([tablesName isEqualToString:@""])) {
if (theResPtr = mysql_list_tables(mConnection, NULL)) {
theResult = [[MCPResult alloc] initWithResPtr: theResPtr encoding: mEncoding timeZone:mTimeZone];
@@ -1803,7 +1831,7 @@ void performThreadedKeepAlive(void *ptr)
}
}
- [queryLock unlock];
+ [self unlockConnection];
if (theResult) {
[theResult autorelease];
@@ -2375,14 +2403,14 @@ void performThreadedKeepAlive(void *ptr)
MCPResult *theResult = nil;
MYSQL_RES *theResPtr;
- [queryLock lock];
+ [self lockConnection];
if (theResPtr = mysql_list_processes(mConnection)) {
theResult = [[MCPResult alloc] initWithResPtr:theResPtr encoding:mEncoding timeZone:mTimeZone];
}
else {
theResult = [[MCPResult alloc] init];
}
- [queryLock unlock];
+ [self unlockConnection];
if (theResult) {
[theResult autorelease];
@@ -2547,7 +2575,7 @@ void performThreadedKeepAlive(void *ptr)
if ([self serverMajorVersion] == 3) queryString = "SHOW VARIABLES LIKE 'max_allowed_packet'";
else queryString = "SELECT @@global.max_allowed_packet";
- [queryLock lock];
+ [self lockConnection];
if (0 == mysql_query(mConnection, queryString)) {
if (mysql_field_count(mConnection) != 0) {
MCPResult *r = [[MCPResult alloc] initWithMySQLPtr:mConnection encoding:mEncoding timeZone:mTimeZone];
@@ -2555,13 +2583,13 @@ void performThreadedKeepAlive(void *ptr)
NSArray *a = [r fetchRowAsArray];
[r autorelease];
if([a count]) {
- [queryLock unlock];
+ [self unlockConnection];
maxAllowedPacketSize = [[a objectAtIndex:([self serverMajorVersion] == 3)?1:0] integerValue];
return true;
}
}
}
- [queryLock unlock];
+ [self unlockConnection];
return false;
}
@@ -2596,9 +2624,9 @@ void performThreadedKeepAlive(void *ptr)
{
if(![self isMaxAllowedPacketEditable] || newSize < 1024) return maxAllowedPacketSize;
- [queryLock lock];
+ [self lockConnection];
mysql_query(mConnection, [[NSString stringWithFormat:@"SET GLOBAL max_allowed_packet = %ld", newSize] UTF8String]);
- [queryLock unlock];
+ [self unlockConnection];
// Inform the user via a log entry about that change according to reset value
if(delegate && [delegate respondsToSelector:@selector(queryGaveError:connection:)])
@@ -2617,9 +2645,9 @@ void performThreadedKeepAlive(void *ptr)
{
BOOL isEditable;
- [queryLock lock];
+ [self lockConnection];
isEditable = !mysql_query(mConnection, "SET GLOBAL max_allowed_packet = @@global.max_allowed_packet");
- [queryLock unlock];
+ [self unlockConnection];
return isEditable;
}
@@ -2753,7 +2781,7 @@ void performThreadedKeepAlive(void *ptr)
// Ensure the query lock is unlocked, thereafter setting to nil in case of pending calls
[self unlockConnection];
- [queryLock release], queryLock = nil;
+ [connectionLock release], connectionLock = nil;
// Clean up connections if necessary
if (mConnected) [self disconnect];
diff --git a/Frameworks/MCPKit/MCPFoundationKit/MCPConstants.h b/Frameworks/MCPKit/MCPFoundationKit/MCPConstants.h
index acdeee16..fba7d651 100644
--- a/Frameworks/MCPKit/MCPFoundationKit/MCPConstants.h
+++ b/Frameworks/MCPKit/MCPFoundationKit/MCPConstants.h
@@ -52,6 +52,13 @@ enum
};
typedef NSUInteger MCPQueryStreamingType;
+// Connection state
+// This is used internally by MCPConnection to prevent simultaneous execution of different queries
+enum {
+ MCPConnectionIdle = 0,
+ MCPConnectionBusy = 1
+};
+
// Charcater set mapping constants
typedef struct _OUR_CHARSET
{
diff --git a/Frameworks/MCPKit/MCPFoundationKit/MCPStreamingResult.m b/Frameworks/MCPKit/MCPFoundationKit/MCPStreamingResult.m
index f6371b03..60917380 100644
--- a/Frameworks/MCPKit/MCPFoundationKit/MCPStreamingResult.m
+++ b/Frameworks/MCPKit/MCPFoundationKit/MCPStreamingResult.m
@@ -85,7 +85,9 @@
[mNames release];
mNames = nil;
}
-
+
+
+
mResult = mysql_use_result(mySQLPtr);
if (mResult) {
@@ -98,7 +100,7 @@
// Obtain SEL references and pointer
isConnectedSEL = @selector(isConnected);
isConnectedPtr = [parentConnection methodForSelector:isConnectedSEL];
-
+
// If the result is opened in download-data-fast safe mode, set up the additional variables
// and threads required.
if (!fullyStreaming) {
@@ -131,9 +133,14 @@
*/
- (void) dealloc
{
- [self cancelResultLoad];
- if (!connectionUnlocked) [parentConnection unlockConnection];
-
+ [self cancelResultLoad]; //this should close the connection if it is still open
+
+ if (!connectionUnlocked) {
+ //this should NEVER happen
+ NSLog(@"MCPStreamingResult: The connection has not been unlocked.");
+ [parentConnection unlockConnection];
+ }
+
if (!fullyStreaming) {
pthread_mutex_destroy(&dataFreeLock);
pthread_mutex_destroy(&dataCreationLock);
@@ -162,8 +169,14 @@
if (fullyStreaming) {
theRow = mysql_fetch_row(mResult);
- // If no data was returned, we're at the end of the result set - return nil.
- if (theRow == NULL) return nil;
+ // If no data was returned, we're at the end of the result set - unlock the connection and return nil
+ if (theRow == NULL) {
+ if (!connectionUnlocked) {
+ [parentConnection unlockConnection];
+ connectionUnlocked = YES;
+ }
+ return nil;
+ }
// Retrieve the lengths of the returned data
fieldLengths = mysql_fetch_lengths(mResult);
@@ -333,6 +346,7 @@
/*
* Ensure the result set is fully processed and freed without any processing
+ * This method ensures that the connection is unlocked.
*/
- (void) cancelResultLoad
{
@@ -345,7 +359,13 @@
theRow = mysql_fetch_row(mResult);
// If no data was returned, we're at the end of the result set - return.
- if (theRow == NULL) return;
+ if (theRow == NULL) {
+ if (!connectionUnlocked) {
+ [parentConnection unlockConnection];
+ connectionUnlocked = YES;
+ }
+ return;
+ }
}
// If in cached-streaming/fast download mode, loop until all data is fetched and freed
@@ -361,8 +381,8 @@
// once all memory has been freed
if (processedRowCount == downloadedRowCount) {
while (!dataFreed) usleep(1000);
- [parentConnection unlockConnection];
- connectionUnlocked = YES;
+ // we don't need to unlock the connection because
+ // the data loading thread already did that
return;
}
processedRowCount++;
@@ -459,8 +479,8 @@
}
// Unlock the parent connection now data has been retrieved
- connectionUnlocked = YES;
- [parentConnection unlockConnection];
+ [parentConnection unlockConnection];
+ connectionUnlocked = YES;
dataDownloaded = YES;
[downloadPool drain];