From d2237c05664461e086664d25fbe717a06e9b5b75 Mon Sep 17 00:00:00 2001 From: rowanbeentje Date: Tue, 15 May 2012 23:40:08 +0000 Subject: Improve connection keepalive, disconnect, and connection loss after reviewing crash logs and testing a number of situations: - Improve stability of closing connections after a connection loss - Minimise prompting a user for connection state restore if closing windows/tabs - Allow cancellation of keepalive ping threads to prevent crashes after deallocation of parent - Manually handle ping thread state struct memory to avoid cross-thread deallocation issues - Improve disconnection speed and resilience --- .../Ping & KeepAlive.h | 3 ++ .../Ping & KeepAlive.m | 63 +++++++++++++++++----- .../SPMySQLFramework/Source/SPMySQLConnection.h | 3 +- .../SPMySQLFramework/Source/SPMySQLConnection.m | 38 ++++++++++--- Source/SPDatabaseDocument.m | 8 +-- Source/SPNavigatorController.m | 1 - 6 files changed, 92 insertions(+), 24 deletions(-) diff --git a/Frameworks/SPMySQLFramework/Source/SPMySQLConnection Categories/Ping & KeepAlive.h b/Frameworks/SPMySQLFramework/Source/SPMySQLConnection Categories/Ping & KeepAlive.h index e84c4ca6..0bbac2d2 100644 --- a/Frameworks/SPMySQLFramework/Source/SPMySQLConnection Categories/Ping & KeepAlive.h +++ b/Frameworks/SPMySQLFramework/Source/SPMySQLConnection Categories/Ping & KeepAlive.h @@ -52,4 +52,7 @@ void _backgroundPingTask(void *ptr); void _forceThreadExit(int signalNumber); void _pingThreadCleanup(void *pingDetails); +// Cancellation +- (void)_cancelKeepAlives; + @end diff --git a/Frameworks/SPMySQLFramework/Source/SPMySQLConnection Categories/Ping & KeepAlive.m b/Frameworks/SPMySQLFramework/Source/SPMySQLConnection Categories/Ping & KeepAlive.m index 9a28bf57..fb8a984f 100644 --- a/Frameworks/SPMySQLFramework/Source/SPMySQLConnection Categories/Ping & KeepAlive.m +++ b/Frameworks/SPMySQLFramework/Source/SPMySQLConnection Categories/Ping & KeepAlive.m @@ -49,7 +49,8 @@ - (void)_keepAlive { - // Do nothing if not connected or if keepalive is disabled + // Do nothing if not connected, if keepalive is disabled, or a keepalive is in + // progress. if (state != SPMySQLConnected || !useKeepAlive) return; // Check to see whether a ping is required. First, compare the last query @@ -81,6 +82,7 @@ */ - (void)_threadedKeepAlive { + keepAliveThread = [NSThread currentThread]; // If the maximum number of ping failures has been reached, determine whether to reconnect. if (keepAliveLastPingBlocked || keepAlivePingFailures >= 3) { @@ -97,6 +99,7 @@ } // Return as no further ping action required this cycle. + keepAliveThread = nil; return; } @@ -107,6 +110,7 @@ } else { keepAlivePingFailures++; } + keepAliveThread = nil; } #pragma mark - @@ -142,16 +146,16 @@ if (timeout > 0) pingTimeout = timeout; // Set up a struct containing details the ping task will need - SPMySQLConnectionPingDetails pingDetails; - pingDetails.mySQLConnection = mySQLConnection; - pingDetails.keepAliveLastPingSuccessPointer = &keepAliveLastPingSuccess; - pingDetails.keepAlivePingActivePointer = &keepAlivePingThreadActive; + SPMySQLConnectionPingDetails *pingDetails = malloc(sizeof(SPMySQLConnectionPingDetails)); + pingDetails->mySQLConnection = mySQLConnection; + pingDetails->keepAliveLastPingSuccessPointer = &keepAliveLastPingSuccess; + pingDetails->keepAlivePingActivePointer = &keepAlivePingThreadActive; // Create a pthread for the ping pthread_attr_t attr; pthread_attr_init(&attr); pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED); - pthread_create(&keepAlivePingThread, &attr, (void *)&_backgroundPingTask, &pingDetails); + pthread_create(&keepAlivePingThread_t, &attr, (void *)&_backgroundPingTask, pingDetails); // Record the ping start time pingStartTime_t = mach_absolute_time(); @@ -161,25 +165,29 @@ usleep((useconds_t)loopDelay); pingElapsedTime = _elapsedSecondsSinceAbsoluteTime(pingStartTime_t); - // If the ping timeout has been exceeded, force a timeout; double-check that the - // thread is still active. - if (pingElapsedTime > pingTimeout && keepAlivePingThreadActive && !threadCancelled) { - pthread_cancel(keepAlivePingThread); + // If the ping timeout has been exceeded, or the ping thread has been + // cancelled, force a timeout; double-check that the thread is still active. + if (([keepAliveThread isCancelled] || pingElapsedTime > pingTimeout) + && keepAlivePingThreadActive + && !threadCancelled) + { + pthread_cancel(keepAlivePingThread_t); threadCancelled = YES; // If the timeout has been exceeded by an additional two seconds, and the thread is // still active, kill the thread. This can occur in certain network conditions causing // a blocking read. } else if (pingElapsedTime > (pingTimeout + 2) && keepAlivePingThreadActive) { - pthread_kill(keepAlivePingThread, SIGUSR1); + pthread_kill(keepAlivePingThread_t, SIGUSR1); keepAlivePingThreadActive = NO; keepAliveLastPingBlocked = YES; } } while (keepAlivePingThreadActive); // Clean up - keepAlivePingThread = NULL; + keepAlivePingThread_t = NULL; pthread_attr_destroy(&attr); + free(pingDetails); // Unlock the connection [self _unlockConnection]; @@ -221,6 +229,11 @@ void _forceThreadExit(int signalNumber) pthread_exit(NULL); } +/** + * A thread cleanup routine. This is added to the thread using a + * pthread_cleanup_push call; a pthread_exit or a pthread_cleanup_pop + * both execute this function. + */ void _pingThreadCleanup(void *pingDetails) { SPMySQLConnectionPingDetails *pingDetailsStruct = pingDetails; @@ -230,4 +243,30 @@ void _pingThreadCleanup(void *pingDetails) mysql_thread_end(); } +#pragma mark - +#pragma mark Cancellation + +/** + * If a keepalive thread is active, cancel it, and wait a short time for it + * to exit. + */ +- (void)_cancelKeepAlives +{ + + // If no keepalive thread is active, return + if (!keepAliveThread) { + return; + } + + // Mark the thread as cancelled + [keepAliveThread cancel]; + + // Wait inside a time limit of ten seconds for it to exit + uint64_t threadCancelStartTime_t = mach_absolute_time(); + do { + usleep(100000); + if (_elapsedSecondsSinceAbsoluteTime(threadCancelStartTime_t) > 10) break; + } while (keepAliveThread); +} + @end diff --git a/Frameworks/SPMySQLFramework/Source/SPMySQLConnection.h b/Frameworks/SPMySQLFramework/Source/SPMySQLConnection.h index a44ae46f..3400ecfd 100644 --- a/Frameworks/SPMySQLFramework/Source/SPMySQLConnection.h +++ b/Frameworks/SPMySQLFramework/Source/SPMySQLConnection.h @@ -86,7 +86,8 @@ CGFloat keepAliveInterval; uint64_t lastKeepAliveTime; NSUInteger keepAlivePingFailures; - pthread_t keepAlivePingThread; + NSThread *keepAliveThread; + pthread_t keepAlivePingThread_t; BOOL keepAlivePingThreadActive; BOOL keepAliveLastPingSuccess; BOOL keepAliveLastPingBlocked; diff --git a/Frameworks/SPMySQLFramework/Source/SPMySQLConnection.m b/Frameworks/SPMySQLFramework/Source/SPMySQLConnection.m index 26276bda..4dbed002 100644 --- a/Frameworks/SPMySQLFramework/Source/SPMySQLConnection.m +++ b/Frameworks/SPMySQLFramework/Source/SPMySQLConnection.m @@ -131,7 +131,8 @@ const char *SPMySQLSSLPermissibleCiphers = "DHE-RSA-AES256-SHA:AES256-SHA:DHE-RS keepAliveInterval = 60; keepAlivePingFailures = 0; lastKeepAliveTime = 0; - keepAlivePingThread = NULL; + keepAliveThread = nil; + keepAlivePingThread_t = NULL; keepAlivePingThreadActive = NO; keepAliveLastPingSuccess = NO; keepAliveLastPingBlocked = NO; @@ -206,6 +207,9 @@ const char *SPMySQLSSLPermissibleCiphers = "DHE-RSA-AES256-SHA:AES256-SHA:DHE-RS [keepAliveTimer invalidate]; [keepAliveTimer release]; + // If a keepalive thread is active, cancel it + [self _cancelKeepAlives]; + // Disconnect if appropriate (which should also disconnect any proxy) [self disconnect]; @@ -326,8 +330,16 @@ const char *SPMySQLSSLPermissibleCiphers = "DHE-RSA-AES256-SHA:AES256-SHA:DHE-RS - (void)disconnect { + // If state is connection lost, set state directly to disconnected. + if (state == SPMySQLConnectionLostInBackground) { + state = SPMySQLDisconnected; + } + // Only continue if a connection is active - if (state != SPMySQLConnected && state != SPMySQLConnecting) return; + if (state != SPMySQLConnected && state != SPMySQLConnecting) { + return; + } + state = SPMySQLDisconnecting; // If a query is active, cancel it @@ -335,12 +347,12 @@ const char *SPMySQLSSLPermissibleCiphers = "DHE-RSA-AES256-SHA:AES256-SHA:DHE-RS // Allow any pings or cancelled queries to complete, inside a time limit of ten seconds uint64_t disconnectStartTime_t = mach_absolute_time(); - do { + while (![self _tryLockConnection]) { usleep(100000); if (_elapsedSecondsSinceAbsoluteTime(disconnectStartTime_t) > 10) break; - } while (![self _tryLockConnection]); + } [self _unlockConnection]; - if (keepAlivePingThread != NULL) pthread_cancel(keepAlivePingThread), keepAlivePingThread = NULL; + [self _cancelKeepAlives]; // 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 @@ -499,6 +511,7 @@ const char *SPMySQLSSLPermissibleCiphers = "DHE-RSA-AES256-SHA:AES256-SHA:DHE-RS */ - (MYSQL *)_makeRawMySQLConnectionWithEncoding:(NSString *)encodingName isMasterConnection:(BOOL)isMaster { + if ([[NSThread currentThread] isCancelled]) return NULL; // Set up the MySQL connection object MYSQL *theConnection = mysql_init(NULL); @@ -625,6 +638,11 @@ const char *SPMySQLSSLPermissibleCiphers = "DHE-RSA-AES256-SHA:AES256-SHA:DHE-RS } } + if ([[NSThread currentThread] isCancelled]) { + [reconnectionPool release]; + return NO; + } + isReconnecting = YES; // Store certain details about the connection, so that if the reconnection is successful @@ -648,6 +666,12 @@ const char *SPMySQLSSLPermissibleCiphers = "DHE-RSA-AES256-SHA:AES256-SHA:DHE-RS // If no network is present, wait for a short time for one to become available [self _waitForNetworkConnectionWithTimeout:10]; + if ([[NSThread currentThread] isCancelled]) { + isReconnecting = NO; + [reconnectionPool release]; + return NO; + } + // If there is a proxy, attempt to reconnect it in blocking fashion if (proxy) { uint64_t loopIterationStart_t, proxyWaitStart_t; @@ -719,7 +743,7 @@ const char *SPMySQLSSLPermissibleCiphers = "DHE-RSA-AES256-SHA:AES256-SHA:DHE-RS } // If the reconnection succeeded, restore the connection state as appropriate - if (state == SPMySQLConnected) { + if (state == SPMySQLConnected && ![[NSThread currentThread] isCancelled]) { reconnectSucceeded = YES; if (databaseToRestore) { [self selectDatabase:databaseToRestore]; @@ -733,7 +757,7 @@ const char *SPMySQLSSLPermissibleCiphers = "DHE-RSA-AES256-SHA:AES256-SHA:DHE-RS // If the connection failed and the connection is permitted to retry, // then retry the reconnection. - } else if (canRetry) { + } else if (canRetry && ![[NSThread currentThread] isCancelled]) { // Default to attempting another reconnect SPMySQLConnectionLostDecision connectionLostDecision = SPMySQLConnectionLostReconnect; diff --git a/Source/SPDatabaseDocument.m b/Source/SPDatabaseDocument.m index 75af2ed1..f8f1585b 100644 --- a/Source/SPDatabaseDocument.m +++ b/Source/SPDatabaseDocument.m @@ -3961,12 +3961,14 @@ static NSString *SPRenameDatabaseAction = @"SPRenameDatabase"; // edits in progress in various views. if ( ![tablesListInstance selectionShouldChangeInTableView:nil] ) return NO; - // Auto-save spf file based connection and return whether the save was successful + // Auto-save spf file based connection and return if the save was not successful if([self fileURL] && [[[self fileURL] path] length] && ![self isUntitled]) { BOOL isSaved = [self saveDocumentWithFilePath:nil inBackground:YES onlyPreferences:YES contextInfo:nil]; - if(isSaved) + if (isSaved) { [[SPQueryController sharedQueryController] removeRegisteredDocumentWithFileURL:[self fileURL]]; - return isSaved; + } else { + return NO; + } } // Terminate all running BASH commands diff --git a/Source/SPNavigatorController.m b/Source/SPNavigatorController.m index bd144e2a..9d1c660a 100644 --- a/Source/SPNavigatorController.m +++ b/Source/SPNavigatorController.m @@ -302,7 +302,6 @@ static NSComparisonResult compareStrings(NSString *s1, NSString *s2, void* conte // If so, don't remove it. if ([[NSApp delegate] frontDocument]) { for(id doc in [[NSApp delegate] orderedDocuments]) { - if(![[doc valueForKeyPath:@"mySQLConnection"] isConnected]) continue; if([[doc connectionID] isEqualToString:connectionID]) docCounter++; if(docCounter > 1) break; -- cgit v1.2.3