Merge pull request #1277 from smartdevicelink/bugfix/issue_1264_file_manager_race_condition

Fixed file manager race condition

2 files changed, 50 insertions, 4 deletions

diff --git a/SmartDeviceLink/SDLFileManager.m b/SmartDeviceLink/SDLFileManager.m
index 911ec889f..eef7e92ac 100644
--- a/SmartDeviceLink/SDLFileManager.m
+++ b/SmartDeviceLink/SDLFileManager.m
@@ -404,8 +404,6 @@ SDLFileManagerState *const SDLFileManagerStateStartupError = @"StartupError";
             if ([self sdl_canFileBeUploadedAgain:file maxUploadCount:maxUploadCount failedFileUploadsCount:self.failedFileUploadsCount]) {
                 SDLLogD(@"Attempting to resend file with name %@ after a failed upload attempt", file.name);
                 return [self sdl_uploadFile:file completionHandler:handler];
-            } else {
-                SDLLogE(@"File named %@ failed to upload. Max number of upload attempts reached", file.name);
             }
         }
 
@@ -526,11 +524,28 @@ SDLFileManagerState *const SDLFileManagerStateStartupError = @"StartupError";
  *  @return                 True if the file still needs to be (re)sent to Core; false if not.
  */
 - (BOOL)sdl_canFileBeUploadedAgain:(nullable SDLFile *)file maxUploadCount:(UInt8)maxUploadCount failedFileUploadsCount:(NSMutableDictionary<SDLFileName *, NSNumber<SDLUInt> *> *)failedFileUploadsCount {
-    if (!file || [self hasUploadedFile:file]) {
+    if (![self.currentState isEqualToString:SDLFileManagerStateReady]) {
+        SDLLogW(@"File named %@ failed to upload. The file manager has shutdown so the file upload will not retry.", file.name);
+        return NO;
+    }
+
+    if (!file) {
+        SDLLogE(@"File can not be uploaded because it is not a valid file.");
+        return NO;
+    }
+
+    if ([self hasUploadedFile:file]) {
+        SDLLogD(@"File named %@ has already been uploaded.", file.name);
         return NO;
     }
+
     NSNumber *failedUploadCount = failedFileUploadsCount[file.name];
-    return (failedUploadCount == nil) ? YES : (failedUploadCount.integerValue < maxUploadCount);
+    BOOL canFileBeUploadedAgain = (failedUploadCount == nil) ? YES : (failedUploadCount.integerValue < maxUploadCount);
+    if (!canFileBeUploadedAgain) {
+        SDLLogE(@"File named %@ failed to upload. Max number of upload attempts reached.", file.name);
+    }
+
+    return canFileBeUploadedAgain;
 }
 
 /**
diff --git a/SmartDeviceLinkTests/DevAPISpecs/SDLFileManagerSpec.m b/SmartDeviceLinkTests/DevAPISpecs/SDLFileManagerSpec.m
index afbd93873..c7dd86483 100644
--- a/SmartDeviceLinkTests/DevAPISpecs/SDLFileManagerSpec.m
+++ b/SmartDeviceLinkTests/DevAPISpecs/SDLFileManagerSpec.m
@@ -1,5 +1,6 @@
 #import <Quick/Quick.h>
 #import <Nimble/Nimble.h>
+#import <OCMock/OCMock.h>
 
 #import "SDLDeleteFileResponse.h"
 #import "SDLError.h"
@@ -14,6 +15,7 @@
 #import "SDLPutFile.h"
 #import "SDLPutFileResponse.h"
 #import "SDLRPCResponse.h"
+#import "SDLStateMachine.h"
 #import "TestConnectionManager.h"
 #import "TestMultipleFilesConnectionManager.h"
 #import "TestFileProgressResponse.h"
@@ -31,6 +33,7 @@ SDLFileManagerState *const SDLFileManagerStateStartupError = @"StartupError";
 @property (strong, nonatomic) NSMutableDictionary<SDLFileName *, NSNumber<SDLUInt> *> *failedFileUploadsCount;
 @property (assign, nonatomic) UInt8 maxFileUploadAttempts;
 @property (assign, nonatomic) UInt8 maxArtworkUploadAttempts;
+@property (strong, nonatomic) SDLStateMachine *stateMachine;
 
 - (BOOL)sdl_canFileBeUploadedAgain:(nullable SDLFile *)file maxUploadCount:(int)maxRetryCount failedFileUploadsCount:(NSMutableDictionary<SDLFileName *, NSNumber<SDLUInt> *> *)failedFileUploadsCount;
 + (NSMutableDictionary<SDLFileName *, NSNumber<SDLUInt> *> *)sdl_incrementFailedUploadCountForFileName:(SDLFileName *)fileName failedFileUploadsCount:(NSMutableDictionary<SDLFileName *, NSNumber<SDLUInt> *> *)failedFileUploadsCount;
@@ -1675,12 +1678,33 @@ describe(@"SDLFileManager reupload failed files", ^{
 
         describe(@"the file cannot be uploaded again", ^{
             it(@"should not upload a file that is nil", ^{
+                // Make sure we are in the ready state
+                testFileManager.stateMachine = OCMClassMock([SDLStateMachine class]);
+                OCMStub([testFileManager.stateMachine currentState]).andReturn(SDLFileManagerStateReady);
+                expect([testFileManager.currentState isEqualToEnum:SDLFileManagerStateReady]).to(beTrue());
+
                 testFile = nil;
                 BOOL canUploadAgain = [testFileManager sdl_canFileBeUploadedAgain:testFile maxUploadCount:5 failedFileUploadsCount:testFailedFileUploadsCount];
                 expect(canUploadAgain).to(equal(NO));
             });
 
             it(@"should not upload a file that has already been uploaded the max number of times", ^{
+                // Make sure we are in the ready state
+                testFileManager.stateMachine = OCMClassMock([SDLStateMachine class]);
+                OCMStub([testFileManager.stateMachine currentState]).andReturn(SDLFileManagerStateReady);
+                expect([testFileManager.currentState isEqualToEnum:SDLFileManagerStateReady]).to(beTrue());
+
+                testFailedFileUploadsCount[testFileName] = @4;
+                BOOL canUploadAgain = [testFileManager sdl_canFileBeUploadedAgain:testFile maxUploadCount:4 failedFileUploadsCount:testFailedFileUploadsCount];
+                expect(canUploadAgain).to(equal(NO));
+            });
+
+            it(@"should not upload a file if the file manager is not in state ready", ^{
+                // Make sure we are NOT in the ready state
+                testFileManager.stateMachine = OCMClassMock([SDLStateMachine class]);
+                OCMStub([testFileManager.stateMachine currentState]).andReturn(SDLFileManagerStateShutdown);
+                expect([testFileManager.currentState isEqualToEnum:SDLFileManagerStateShutdown]).to(beTrue());
+
                 testFailedFileUploadsCount[testFileName] = @4;
                 BOOL canUploadAgain = [testFileManager sdl_canFileBeUploadedAgain:testFile maxUploadCount:4 failedFileUploadsCount:testFailedFileUploadsCount];
                 expect(canUploadAgain).to(equal(NO));
@@ -1688,6 +1712,13 @@ describe(@"SDLFileManager reupload failed files", ^{
         });
 
         describe(@"the file can be uploaded again", ^{
+            beforeEach(^{
+                // Make sure we are in the ready state
+                testFileManager.stateMachine = OCMClassMock([SDLStateMachine class]);
+                OCMStub([testFileManager.stateMachine currentState]).andReturn(SDLFileManagerStateReady);
+                expect([testFileManager.currentState isEqualToEnum:SDLFileManagerStateReady]).to(beTrue());
+            });
+
             it(@"should upload a file that has not yet failed to upload", ^{
                 testFailedFileUploadsCount = [NSMutableDictionary dictionary];
                 BOOL canUploadAgain = [testFileManager sdl_canFileBeUploadedAgain:testFile maxUploadCount:2 failedFileUploadsCount:testFailedFileUploadsCount];