Merge pull request #1011 from Lievesley/bugfix/issue_1002_sdk_hangs_on_disconnection

Prevent thread deadlock when disconnecting the iPhone from the Head Unit

2 files changed, 32 insertions, 25 deletions

diff --git a/SmartDeviceLink/SDLIAPSession.m b/SmartDeviceLink/SDLIAPSession.m
index 47047ece9..98cc35813 100644
--- a/SmartDeviceLink/SDLIAPSession.m
+++ b/SmartDeviceLink/SDLIAPSession.m
@@ -11,7 +11,7 @@
 NS_ASSUME_NONNULL_BEGIN
 
 NSString *const IOStreamThreadName = @"com.smartdevicelink.iostream";
-NSTimeInterval const StreamThreadWaitSecs = 1.0;
+NSTimeInterval const StreamThreadWaitSecs = 10.0;
 
 @interface SDLIAPSession ()
 
@@ -80,7 +80,6 @@ NSTimeInterval const StreamThreadWaitSecs = 1.0;
 }
 
 - (void)stop {
-    // This method must be called on the main thread
     if ([NSThread isMainThread]) {
         [self sdl_stop];
     } else {
@@ -91,6 +90,7 @@ NSTimeInterval const StreamThreadWaitSecs = 1.0;
 }
 
 - (void)sdl_stop {
+    NSAssert(NSThread.isMainThread, @"%@ must only be called on the main thread", NSStringFromSelector(_cmd));
     if (self.isDataSession) {
         [self.ioStreamThread cancel];
 
@@ -137,6 +137,9 @@ NSTimeInterval const StreamThreadWaitSecs = 1.0;
         if (bytesWritten < 0) {
             if (ostream.streamError != nil) {
                 [self sdl_handleOutputStreamWriteError:ostream.streamError];
+            } else {
+                // The write operation failed but there is no further information about the error. This can occur when disconnecting from an external accessory.
+                SDLLogE(@"Output stream write operation failed");
             }
         } else if (bytesWritten == bytesRemaining) {
             // Remove the data from the queue
@@ -173,13 +176,10 @@ NSTimeInterval const StreamThreadWaitSecs = 1.0;
         [self startStream:self.easession.outputStream];
 
         SDLLogD(@"Starting the accessory event loop");
-        do {
-            if (self.sendDataQueue.count > 0 && !self.sendDataQueue.frontDequeued) {
-                [self sdl_dequeueAndWriteToOutputStream];
-            }
-            // The principle here is to give the event loop enough time to process stream events while also allowing it to handle new enqueued data buffers in a timely manner. We're capping the run loop CPU time at 0.25s maximum before it will return control to the rest of the loop.
+        while (!NSThread.currentThread.cancelled) {
+            // Enqueued data will be written to and read from the streams in the runloop
             [[NSRunLoop currentRunLoop] runMode:NSDefaultRunLoopMode beforeDate:[NSDate dateWithTimeIntervalSinceNow:0.25f]];
-        } while (![NSThread currentThread].cancelled);
+        }
 
         SDLLogD(@"Closing the accessory session for id: %tu, name: %@", self.easession.accessory.connectionID, self.easession.accessory.name);
 
diff --git a/SmartDeviceLink/SDLIAPTransport.m b/SmartDeviceLink/SDLIAPTransport.m
index df623d980..5283ae652 100644
--- a/SmartDeviceLink/SDLIAPTransport.m
+++ b/SmartDeviceLink/SDLIAPTransport.m
@@ -549,28 +549,33 @@ int const ProtocolIndexTimeoutSeconds = 10;
 
 - (SDLStreamEndHandler)sdl_dataStreamEndedHandler {
     __weak typeof(self) weakSelf = self;
-    
     return ^(NSStream *stream) {
+        NSAssert(!NSThread.isMainThread, @"%@ should only be called on the IO thread", NSStringFromSelector(_cmd));
         __strong typeof(weakSelf) strongSelf = weakSelf;
+
         SDLLogD(@"Data stream ended");
-        if (strongSelf.session != nil) {
-            // The handler will be called on the IO thread, but the session stop method must be called on the main thread and we need to wait for the session to stop before nil'ing it out. To do this, we use dispatch_sync() on the main thread.
-            dispatch_sync(dispatch_get_main_queue(), ^{
-                [strongSelf.session stop];
-            });
+        if (strongSelf.session == nil) {
+            SDLLogD(@"Data session is nil");
+            return;
+        }
+        // The handler will be called on the IO thread, but the session stop method must be called on the main thread
+        dispatch_async(dispatch_get_main_queue(), ^{
+            [strongSelf.session stop];
             strongSelf.session.streamDelegate = nil;
             strongSelf.session = nil;
-        }
-        // We don't call sdl_retryEstablishSession here because the stream end event usually fires when the accessory is disconnected
+            
+            // We don't call sdl_retryEstablishSession here because the stream end event usually fires when the accessory is disconnected
+        });
+        
+        // To prevent deadlocks the handler must return to the runloop and not block the thread
     };
 }
 
 - (SDLStreamHasBytesHandler)sdl_dataStreamHasBytesHandler {
     __weak typeof(self) weakSelf = self;
-    
     return ^(NSInputStream *istream) {
+        NSAssert(!NSThread.isMainThread, @"%@ should only be called on the IO thread", NSStringFromSelector(_cmd));
         __strong typeof(weakSelf) strongSelf = weakSelf;
-        
         uint8_t buf[[[SDLGlobals sharedGlobals] mtuSizeForServiceType:SDLServiceTypeRPC]];
         while (istream.streamStatus == NSStreamStatusOpen && istream.hasBytesAvailable) {
             // It is necessary to check the stream status and whether there are bytes available because the dataStreamHasBytesHandler is executed on the IO thread and the accessory disconnect notification arrives on the main thread, causing data to be passed to the delegate while the main thread is tearing down the transport.
@@ -597,18 +602,20 @@ int const ProtocolIndexTimeoutSeconds = 10;
 
 - (SDLStreamErrorHandler)sdl_dataStreamErroredHandler {
     __weak typeof(self) weakSelf = self;
-    
     return ^(NSStream *stream) {
+        NSAssert(!NSThread.isMainThread, @"%@ should only be called on the IO thread", NSStringFromSelector(_cmd));
         __strong typeof(weakSelf) strongSelf = weakSelf;
         SDLLogE(@"Data stream error");
-        dispatch_sync(dispatch_get_main_queue(), ^{
+        dispatch_async(dispatch_get_main_queue(), ^{
             [strongSelf.session stop];
+            strongSelf.session.streamDelegate = nil;
+            strongSelf.session = nil;
+            if (![LegacyProtocolString isEqualToString:strongSelf.session.protocol]) {
+                [strongSelf sdl_retryEstablishSession];
+            }
         });
-        strongSelf.session.streamDelegate = nil;
-        strongSelf.session = nil;
-        if (![LegacyProtocolString isEqualToString:strongSelf.session.protocol]) {
-            [strongSelf sdl_retryEstablishSession];
-        }
+        
+        // To prevent deadlocks the handler must return to the runloop and not block the thread
     };
 }