Refactor response dispatcher threadingbugfix/issue-1559-fix-APT-removal-after-handler-called

* All getters for the properties now use a dispatch_sync
* Now runs on the serial processing queue instead of the concurrent processing queue
* Reverted most of the tests to only check the properties serially since they're dispatch_sync'd now

2 files changed, 170 insertions, 93 deletions

diff --git a/SmartDeviceLink/SDLResponseDispatcher.m b/SmartDeviceLink/SDLResponseDispatcher.m
index 280ae14f7..7c5956616 100644
--- a/SmartDeviceLink/SDLResponseDispatcher.m
+++ b/SmartDeviceLink/SDLResponseDispatcher.m
@@ -42,6 +42,11 @@ NS_ASSUME_NONNULL_BEGIN
 
 @property (copy, nonatomic) dispatch_queue_t readWriteQueue;
 
+@property (strong, nonatomic, readwrite) NSMapTable<SDLRPCCorrelationId *, SDLResponseHandler> *rpcResponseHandlerMap;
+@property (strong, nonatomic, readwrite) NSMutableDictionary<SDLRPCCorrelationId *, SDLRPCRequest *> *rpcRequestDictionary;
+@property (strong, nonatomic, readwrite) NSMapTable<SDLAddCommandCommandId *, SDLRPCCommandNotificationHandler> *commandHandlerMap;
+@property (strong, nonatomic, readwrite) NSMapTable<SDLSubscribeButtonName *, SDLRPCButtonNotificationHandler> *buttonHandlerMap;
+@property (strong, nonatomic, readwrite) NSMapTable<SDLSoftButtonId *, SDLRPCButtonNotificationHandler> *customButtonHandlerMap;
 @property (strong, nonatomic, readwrite, nullable) SDLAudioPassThruHandler audioPassThruHandler;
 
 @end
@@ -61,9 +66,9 @@ NS_ASSUME_NONNULL_BEGIN
     }
 
     if (@available(iOS 10.0, *)) {
-        _readWriteQueue = dispatch_queue_create_with_target("com.sdl.lifecycle.responseDispatcher", DISPATCH_QUEUE_CONCURRENT, [SDLGlobals sharedGlobals].sdlConcurrentQueue);
+        _readWriteQueue = dispatch_queue_create_with_target("com.sdl.lifecycle.responseDispatcher", DISPATCH_QUEUE_SERIAL, [SDLGlobals sharedGlobals].sdlProcessingQueue);
     } else {
-        _readWriteQueue = [SDLGlobals sharedGlobals].sdlConcurrentQueue;
+        _readWriteQueue = [SDLGlobals sharedGlobals].sdlProcessingQueue;
     }
 
     _rpcResponseHandlerMap = [NSMapTable mapTableWithKeyOptions:NSMapTableCopyIn valueOptions:NSMapTableCopyIn];
@@ -95,31 +100,33 @@ NS_ASSUME_NONNULL_BEGIN
 #pragma mark - Storage
 
 - (void)storeRequest:(SDLRPCRequest *)request handler:(nullable SDLResponseHandler)handler {
-    NSNumber *correlationId = request.correlationID;
     __weak typeof(self) weakself = self;
+    NSNumber *correlationId = request.correlationID;
 
     // Check for RPCs that require an extra handler
     if ([request isKindOfClass:[SDLAddCommand class]]) {
         SDLAddCommand *addCommand = (SDLAddCommand *)request;
-        if (!addCommand.cmdID) {
+        if (addCommand.cmdID == nil) {
             @throw [NSException sdl_missingIdException];
         }
         if (addCommand.handler != nil) {
-            dispatch_barrier_async(self.readWriteQueue, ^{
-                weakself.commandHandlerMap[addCommand.cmdID] = addCommand.handler;
-            });
+            [self sdl_runAsyncOnQueue:^{
+                __strong typeof(weakself) strongself = weakself;
+                strongself->_commandHandlerMap[addCommand.cmdID] = addCommand.handler;
+            }];
         }
     } else if ([request isKindOfClass:[SDLSubscribeButton class]]) {
         // Convert SDLButtonName to NSString, since it doesn't conform to <NSCopying>
         SDLSubscribeButton *subscribeButton = (SDLSubscribeButton *)request;
         SDLButtonName buttonName = subscribeButton.buttonName;
-        if (!buttonName) {
+        if (buttonName == nil) {
             @throw [NSException sdl_missingIdException];
         }
-        if (subscribeButton.handler) {
-            dispatch_barrier_async(self.readWriteQueue, ^{
-                weakself.buttonHandlerMap[buttonName] = subscribeButton.handler;
-            });
+        if (subscribeButton.handler != nil) {
+           [self sdl_runAsyncOnQueue:^{
+                __strong typeof(weakself) strongself = weakself;
+                strongself->_buttonHandlerMap[buttonName] = subscribeButton.handler;
+            }];
         }
     } else if ([request isKindOfClass:[SDLAlert class]]) {
         SDLAlert *alert = (SDLAlert *)request;
@@ -133,18 +140,21 @@ NS_ASSUME_NONNULL_BEGIN
     } else if ([request isKindOfClass:[SDLPerformAudioPassThru class]]) {
         SDLPerformAudioPassThru *performAudioPassThru = (SDLPerformAudioPassThru *)request;
 
-        dispatch_barrier_async(self.readWriteQueue, ^{
-            weakself.audioPassThruHandler = performAudioPassThru.audioDataHandler;
-        });
+        [self sdl_runAsyncOnQueue:^{
+            __strong typeof(weakself) strongself = weakself;
+            strongself->_audioPassThruHandler = performAudioPassThru.audioDataHandler;
+        }];
     }
 
     // Always store the request, it's needed in some cases whether or not there was a handler (e.g. DeleteCommand).
-    dispatch_barrier_async(self.readWriteQueue, ^{
-        weakself.rpcRequestDictionary[correlationId] = request;
-        if (handler) {
-            weakself.rpcResponseHandlerMap[correlationId] = handler;
+    [self sdl_runAsyncOnQueue:^{
+        __strong typeof(weakself) strongself = weakself;
+
+        strongself->_rpcRequestDictionary[correlationId] = request;
+        if (handler != nil) {
+            strongself->_rpcResponseHandlerMap[correlationId] = handler;
         }
-    });
+    }];
 }
 
 - (void)clear {
@@ -152,15 +162,15 @@ NS_ASSUME_NONNULL_BEGIN
 
     __block NSArray<SDLResponseHandler> *handlers = nil;
     __block NSArray<SDLRPCRequest *> *requests = nil;
-
-    dispatch_sync(self.readWriteQueue, ^{
+    [self sdl_runSyncOnQueue:^{
+        __strong typeof(weakself) strongself = weakself;
         NSMutableArray *handlerArray = [NSMutableArray array];
         NSMutableArray *requestArray = [NSMutableArray array];
 
         // When we get disconnected we have to delete all existing responseHandlers as they are not valid anymore
-        for (SDLRPCCorrelationId *correlationID in self.rpcResponseHandlerMap.dictionaryRepresentation) {
-            SDLResponseHandler responseHandler = self.rpcResponseHandlerMap[correlationID];
-            SDLRPCRequest *request = self.rpcRequestDictionary[correlationID];
+        for (SDLRPCCorrelationId *correlationID in strongself->_rpcResponseHandlerMap.dictionaryRepresentation) {
+            SDLResponseHandler responseHandler = strongself->_rpcResponseHandlerMap[correlationID];
+            SDLRPCRequest *request = strongself->_rpcRequestDictionary[correlationID];
 
             if (responseHandler != NULL) {
                 [handlerArray addObject:responseHandler];
@@ -170,7 +180,7 @@ NS_ASSUME_NONNULL_BEGIN
 
         handlers = [handlerArray copy];
         requests = [requestArray copy];
-    });
+    }];
 
     for (NSUInteger i = 0; i < handlers.count; i++) {
         SDLResponseHandler responseHandler = handlers[i];
@@ -179,27 +189,30 @@ NS_ASSUME_NONNULL_BEGIN
         responseHandler(request, nil, [NSError sdl_lifecycle_notConnectedError]);
     }
 
-    dispatch_barrier_async(self.readWriteQueue, ^{
-        [weakself.rpcRequestDictionary removeAllObjects];
-        [weakself.rpcResponseHandlerMap removeAllObjects];
-        [weakself.commandHandlerMap removeAllObjects];
-        [weakself.buttonHandlerMap removeAllObjects];
-        [weakself.customButtonHandlerMap removeAllObjects];
-        weakself.audioPassThruHandler = nil;
-    });
+    [self sdl_runAsyncOnQueue:^{
+        __strong typeof(weakself) strongself = weakself;
+
+        [strongself->_rpcRequestDictionary removeAllObjects];
+        [strongself->_rpcResponseHandlerMap removeAllObjects];
+        [strongself->_commandHandlerMap removeAllObjects];
+        [strongself->_buttonHandlerMap removeAllObjects];
+        [strongself->_customButtonHandlerMap removeAllObjects];
+        strongself->_audioPassThruHandler = nil;
+    }];
 }
 
 - (void)sdl_addToCustomButtonHandlerMap:(NSArray<SDLSoftButton *> *)softButtons {
     __weak typeof(self) weakself = self;
-
     for (SDLSoftButton *sb in softButtons) {
-        if (!sb.softButtonID) {
+        if (sb.softButtonID == nil) {
             @throw [NSException sdl_missingIdException];
         }
-        if (sb.handler) {
-            dispatch_barrier_async(self.readWriteQueue, ^{
-                weakself.customButtonHandlerMap[sb.softButtonID] = sb.handler;
-            });
+
+        if (sb.handler != nil) {
+            [self sdl_runAsyncOnQueue:^{
+                __strong typeof(weakself) strongself = weakself;
+                strongself->_customButtonHandlerMap[sb.softButtonID] = sb.handler;
+            }];
         }
     }
 }
@@ -223,15 +236,16 @@ NS_ASSUME_NONNULL_BEGIN
 
     __block SDLResponseHandler handler = nil;
     __block SDLRPCRequest *request = nil;
-    dispatch_sync(self.readWriteQueue, ^{
-        handler = self.rpcResponseHandlerMap[response.correlationID];
-        request = self.rpcRequestDictionary[response.correlationID];
-    });
+    [self sdl_runSyncOnQueue:^{
+        handler = self->_rpcResponseHandlerMap[response.correlationID];
+        request = self->_rpcRequestDictionary[response.correlationID];
+    }];
 
     // Find the appropriate request completion handler, remove the request and response handler
-    dispatch_barrier_async(self.readWriteQueue, ^{
-        [weakself.rpcRequestDictionary safeRemoveObjectForKey:response.correlationID];
-        [weakself.rpcResponseHandlerMap safeRemoveObjectForKey:response.correlationID];
+    [self sdl_runAsyncOnQueue:^{
+        __strong typeof(weakself) strongself = weakself;
+        [strongself->_rpcRequestDictionary safeRemoveObjectForKey:response.correlationID];
+        [strongself->_rpcResponseHandlerMap safeRemoveObjectForKey:response.correlationID];
 
         // If we errored on the response, the delete / unsubscribe was unsuccessful
         if (error == nil) {
@@ -239,13 +253,13 @@ NS_ASSUME_NONNULL_BEGIN
             if ([response isKindOfClass:[SDLDeleteCommandResponse class]]) {
                 SDLDeleteCommand *deleteCommandRequest = (SDLDeleteCommand *)request;
                 NSNumber *deleteCommandId = deleteCommandRequest.cmdID;
-                [weakself.commandHandlerMap safeRemoveObjectForKey:deleteCommandId];
+                [strongself->_commandHandlerMap safeRemoveObjectForKey:deleteCommandId];
             } else if ([response isKindOfClass:[SDLUnsubscribeButtonResponse class]]) {
                 SDLUnsubscribeButton *unsubscribeButtonRequest = (SDLUnsubscribeButton *)request;
                 SDLButtonName unsubscribeButtonName = unsubscribeButtonRequest.buttonName;
-                [weakself.buttonHandlerMap safeRemoveObjectForKey:unsubscribeButtonName];
+                [strongself->_buttonHandlerMap safeRemoveObjectForKey:unsubscribeButtonName];
             } else if ([response isKindOfClass:[SDLPerformAudioPassThruResponse class]]) {
-                weakself.audioPassThruHandler = nil;
+                strongself->_audioPassThruHandler = nil;
             }
         }
 
@@ -258,7 +272,7 @@ NS_ASSUME_NONNULL_BEGIN
                 handler(request, response, error);
             }
         });
-    });
+    }];
 }
 
 #pragma mark Command
@@ -266,13 +280,8 @@ NS_ASSUME_NONNULL_BEGIN
 - (void)sdl_runHandlerForCommand:(SDLRPCNotificationNotification *)notification {
     SDLOnCommand *onCommandNotification = notification.notification;
 
-    __block SDLRPCCommandNotificationHandler handler = nil;
-
-    dispatch_sync(self.readWriteQueue, ^{
-        handler = self.commandHandlerMap[onCommandNotification.cmdID];
-    });
-
-    if (handler) {
+    SDLRPCCommandNotificationHandler handler = self.commandHandlerMap[onCommandNotification.cmdID];
+    if (handler != nil) {
         handler(onCommandNotification);
     }
 }
@@ -283,7 +292,6 @@ NS_ASSUME_NONNULL_BEGIN
     __kindof SDLRPCNotification *rpcNotification = notification.notification;
     SDLButtonName name = nil;
     NSNumber *customID = nil;
-    __block SDLRPCButtonNotificationHandler handler = nil;
 
     if ([rpcNotification isMemberOfClass:[SDLOnButtonEvent class]]) {
         name = ((SDLOnButtonEvent *)rpcNotification).buttonName;
@@ -295,15 +303,14 @@ NS_ASSUME_NONNULL_BEGIN
         return;
     }
 
-    dispatch_sync(self.readWriteQueue, ^{
-        if ([name isEqualToEnum:SDLButtonNameCustomButton]) {
-            // Custom buttons
-            handler = self.customButtonHandlerMap[customID];
-        } else {
-            // Static buttons
-            handler = self.buttonHandlerMap[name];
-        }
-    });
+    SDLRPCButtonNotificationHandler handler = nil;
+    if ([name isEqualToEnum:SDLButtonNameCustomButton]) {
+        // Custom buttons
+        handler = self.customButtonHandlerMap[customID];
+    } else {
+        // Static buttons
+        handler = self.buttonHandlerMap[name];
+    }
 
     if (handler == nil) {
         return;
@@ -321,16 +328,86 @@ NS_ASSUME_NONNULL_BEGIN
 - (void)sdl_runHandlerForAudioPassThru:(SDLRPCNotificationNotification *)notification {
     SDLOnAudioPassThru *onAudioPassThruNotification = notification.notification;
 
-    __block SDLAudioPassThruHandler handler = nil;
-    dispatch_sync(self.readWriteQueue, ^{
-        handler = self.audioPassThruHandler;
-    });
-
-    if (handler) {
+    SDLAudioPassThruHandler handler = self.audioPassThruHandler;
+    if (handler != nil) {
         handler(onAudioPassThruNotification.bulkData);
     }
 }
 
+#pragma mark Utilities
+
+- (void)sdl_runSyncOnQueue:(void (^)(void))block {
+    if (dispatch_get_specific(SDLProcessingQueueName) != nil) {
+        block();
+    } else {
+        dispatch_sync(self.readWriteQueue, block);
+    }
+}
+
+- (void)sdl_runAsyncOnQueue:(void (^)(void))block {
+    if (dispatch_get_specific(SDLProcessingQueueName) != nil) {
+        block();
+    } else {
+        dispatch_async(self.readWriteQueue, block);
+    }
+}
+
+#pragma mark Getters
+
+- (NSMapTable<SDLRPCCorrelationId *, SDLResponseHandler> *)rpcResponseHandlerMap {
+    __block NSMapTable<SDLRPCCorrelationId *, SDLResponseHandler> *map = nil;
+    [self sdl_runSyncOnQueue:^{
+        map = self->_rpcResponseHandlerMap;
+    }];
+
+    return map;
+}
+
+- (NSMutableDictionary<SDLRPCCorrelationId *, SDLRPCRequest *> *)rpcRequestDictionary {
+    __block NSMutableDictionary<SDLRPCCorrelationId *, SDLRPCRequest *> *dict = nil;
+    [self sdl_runSyncOnQueue:^{
+        dict = self->_rpcRequestDictionary;
+    }];
+
+    return dict;
+}
+
+- (NSMapTable<SDLAddCommandCommandId *, SDLRPCCommandNotificationHandler> *)commandHandlerMap {
+    __block NSMapTable<SDLAddCommandCommandId *, SDLRPCCommandNotificationHandler> *map = nil;
+    [self sdl_runSyncOnQueue:^{
+        map = self->_commandHandlerMap;
+    }];
+
+    return map;
+}
+
+- (NSMapTable<SDLSubscribeButtonName *, SDLRPCButtonNotificationHandler> *)buttonHandlerMap {
+    __block NSMapTable<SDLSubscribeButtonName *, SDLRPCButtonNotificationHandler> *map = nil;
+    [self sdl_runSyncOnQueue:^{
+        map = self->_buttonHandlerMap;
+    }];
+
+    return map;
+}
+
+- (NSMapTable<SDLSoftButtonId *, SDLRPCButtonNotificationHandler> *)customButtonHandlerMap {
+    __block NSMapTable<SDLSoftButtonId *, SDLRPCButtonNotificationHandler> *map = nil;
+    [self sdl_runSyncOnQueue:^{
+        map = self->_customButtonHandlerMap;
+    }];
+
+    return map;
+}
+
+- (nullable SDLAudioPassThruHandler)audioPassThruHandler {
+    __block SDLAudioPassThruHandler audioPassThruHandler = nil;
+    [self sdl_runSyncOnQueue:^{
+        audioPassThruHandler = self->_audioPassThruHandler;
+    }];
+
+    return audioPassThruHandler;
+}
+
 @end
 
 NS_ASSUME_NONNULL_END
diff --git a/SmartDeviceLinkTests/Notifications/SDLResponseDispatcherSpec.m b/SmartDeviceLinkTests/Notifications/SDLResponseDispatcherSpec.m
index d53fdf294..dc713cb06 100644
--- a/SmartDeviceLinkTests/Notifications/SDLResponseDispatcherSpec.m
+++ b/SmartDeviceLinkTests/Notifications/SDLResponseDispatcherSpec.m
@@ -90,11 +90,11 @@ describe(@"a response dispatcher", ^{
         });
         
         it(@"should store the request and response", ^{
-            expect(testDispatcher.rpcRequestDictionary[testCorrelationId]).toEventuallyNot(beNil());
-            expect(testDispatcher.rpcRequestDictionary).toEventually(haveCount(@1));
+            expect(testDispatcher.rpcRequestDictionary[testCorrelationId]).toNot(beNil());
+            expect(testDispatcher.rpcRequestDictionary).to(haveCount(@1));
             
-            expect(testDispatcher.rpcResponseHandlerMap[testCorrelationId]).toEventuallyNot(beNil());
-            expect(testDispatcher.rpcResponseHandlerMap).toEventually(haveCount(@1));
+            expect(testDispatcher.rpcResponseHandlerMap[testCorrelationId]).toNot(beNil());
+            expect(testDispatcher.rpcResponseHandlerMap).to(haveCount(@1));
         });
         
         describe(@"when a response arrives", ^{
@@ -109,7 +109,7 @@ describe(@"a response dispatcher", ^{
             });
             
             it(@"should run the handler", ^{
-                expect(@(handlerCalled)).to(beTrue());
+                expect(@(handlerCalled)).toEventually(beTrue());
                 expect(testDispatcher.rpcRequestDictionary).to(haveCount(@0));
                 expect(testDispatcher.rpcResponseHandlerMap).to(haveCount(@0));
             });
@@ -139,8 +139,8 @@ describe(@"a response dispatcher", ^{
             });
             
             it(@"should add the soft button to the map", ^{
-                expect(testDispatcher.customButtonHandlerMap[testSoftButton1.softButtonID]).toEventuallyNot(beNil());
-                expect(testDispatcher.customButtonHandlerMap).toEventually(haveCount(@1));
+                expect(testDispatcher.customButtonHandlerMap[testSoftButton1.softButtonID]).toNot(beNil());
+                expect(testDispatcher.customButtonHandlerMap).to(haveCount(@1));
             });
             
             describe(@"when button press and button event notifications arrive", ^{
@@ -249,8 +249,8 @@ describe(@"a response dispatcher", ^{
             it(@"should add the command to the map", ^{
                 [testDispatcher storeRequest:testAddCommand handler:nil];
                 
-                expect(testDispatcher.commandHandlerMap[testAddCommand.cmdID]).toEventuallyNot(beNil());
-                expect(testDispatcher.commandHandlerMap).toEventually(haveCount(@1));
+                expect(testDispatcher.commandHandlerMap[testAddCommand.cmdID]).toNot(beNil());
+                expect(testDispatcher.commandHandlerMap).to(haveCount(@1));
             });
             
             it(@"should throw an exception if there's no command id", ^{
@@ -330,10 +330,10 @@ describe(@"a response dispatcher", ^{
                 
                 it(@"should have removed all the handlers", ^{
                     // There should still be the add command request & handler in the dictionaries since we never responded to those RPCs, but the command handler map should have removed the addCommand handler
-                    expect(testDispatcher.commandHandlerMap).toEventually(haveCount(0));
-                    expect(testDispatcher.rpcRequestDictionary.allKeys).toEventually(haveCount(1));
-                    expect(testDispatcher.rpcResponseHandlerMap).toEventually(haveCount(1));
-                    expect(deleteCommandHandlerMapCount).toEventually(equal(0));
+                    expect(testDispatcher.commandHandlerMap).to(haveCount(0));
+                    expect(testDispatcher.rpcRequestDictionary.allKeys).to(haveCount(1));
+                    expect(testDispatcher.rpcResponseHandlerMap).to(haveCount(1));
+                    expect(deleteCommandHandlerMapCount).to(equal(0));
                 });
             });
         });
@@ -372,8 +372,8 @@ describe(@"a response dispatcher", ^{
             it(@"should add the subscription to the map", ^{
                 [testDispatcher storeRequest:testSubscribeButton handler:nil];
                 
-                expect(testDispatcher.buttonHandlerMap[testSubscribeButton.buttonName]).toEventuallyNot(beNil());
-                expect(testDispatcher.buttonHandlerMap).toEventually(haveCount(@1));
+                expect(testDispatcher.buttonHandlerMap[testSubscribeButton.buttonName]).toNot(beNil());
+                expect(testDispatcher.buttonHandlerMap).to(haveCount(@1));
             });
             
             it(@"should throw an exception if there's no button name", ^{
@@ -502,8 +502,8 @@ describe(@"a response dispatcher", ^{
             });
             
             it(@"should add the soft button to the map", ^{
-                expect(testDispatcher.customButtonHandlerMap[testSoftButton1.softButtonID]).toEventuallyNot(beNil());
-                expect(testDispatcher.customButtonHandlerMap).toEventually(haveCount(@1));
+                expect(testDispatcher.customButtonHandlerMap[testSoftButton1.softButtonID]).toNot(beNil());
+                expect(testDispatcher.customButtonHandlerMap).to(haveCount(@1));
             });
             
             describe(@"when button press and button event notifications arrive", ^{
@@ -613,8 +613,8 @@ describe(@"a response dispatcher", ^{
             });
             
             it(@"should add the soft button to the map", ^{
-                expect(testDispatcher.customButtonHandlerMap[testSoftButton1.softButtonID]).toEventuallyNot(beNil());
-                expect(testDispatcher.customButtonHandlerMap).toEventually(haveCount(@1));
+                expect(testDispatcher.customButtonHandlerMap[testSoftButton1.softButtonID]).toNot(beNil());
+                expect(testDispatcher.customButtonHandlerMap).to(haveCount(@1));
             });
             
             describe(@"when button press and button event notifications arrive", ^{