AWSS3TransferUtility download progress when coming back to foreground

[mrikh]

Describe the bug:

The bug seems to be that we don't get callbacks for the progress continuously if we start the download, put the application in the background and then come back to the foreground. The download does keep on happening just the progress callbacks aren't received.

I am sure the download is happening in the background as the UI gets updated once the download is complete but not while the progress is happening after coming back into the foreground.

I tried debugging the issue, it seems that the callback doesn't happen in the NSURLSession delegate itself inside the SDK until the download is completed. This is the method i'm using for the download:

Edit: I also get a callback in the progress closure just after the coming back into the foreground and just before the download is completed. I've also implemented the appropriate method inside app delegate as well

Before putting app in the background the fraction completed shows values starting from lets say 0 to 0.5 perfectly fine with the appropriate progress values, if i put the app in the background at this point and come back again, the next time i receive the progress will be just before the download is completed and that too only once.

static let utility = AWSS3TransferUtility.default()

static func downloadFileFromAWS(_ fileName : String, progress: AWSS3TransferUtilityProgressBlock?, completion: AWSS3TransferUtilityDownloadCompletionHandlerBlock?, transferTask : ((AWSS3TransferUtilityDownloadTask?, Error?)->())?){

        utility.configuration.maxRetryCount = 2

        let expression = AWSS3TransferUtilityDownloadExpression()
        expression.progressBlock = progress
        
        utility.downloadData(fromBucket: AWSKeys.bucket_name, key: fileName, expression: expression, completionHandler: completion).continueWith { (task) -> Any? in

            DispatchQueue.main.async {
                transferTask?(task.result, task.error)
            }

            return nil
        }
    }

To Reproduce
Steps to reproduce the behavior:
Use the above method to start the download after passing the appropriate callbacks. Put the application in the background and then back into the foreground. Observe that the progress closure isn't called but once the download is done, the completion is called.

Expected behavior
Continue to get progress callbacks.

Screenshots
Can't upload screenshots/video cause its an official project.

Environment(please complete the following information):

• SDK Version: Latest installed via cocoapods
• Dependency Manager: Cocoapods
• Swift Version : 4.2

Device Information (please complete the following information):

• Device: iPhone 7
• iOS Version: 12.1
• Specific to simulators: Nope, haven't run it on the simulator.

Additional context
Add any other context about the problem here.

[scb01]

@mrikh

Thanks for reporting this - I have observed this in my testing as well and had the same understanding as you i.e, the progress update callback in the NSURLSession delegate is invoked at different times as determined by the OS. I have previously looked at NSURLSession options and didn't find any ways of configuring it so that the OS will report progress more often. Are you aware of any configuration options that will help?

[mrikh]

I thought there might be something wrong with my implementation and was mainly focusing on the online AWS docs and its proper implementation rather than the NSURLSession callbacks.

Ahh unfortunately i don't. My knowledge regarding NSURLSession isn't all that extensive apart from the basics. Additionally i tried enabling the background modes and testing on the off chance that it helps but sadly that didn't help at all. I'll try and find out more regarding the background download callbacks from iOS when i get time later on.

[scb01]

@mrikh
Sounds good. As the transfers are being completed and you are getting a final progress update, I will go ahead and close this issue for now as it working as expected. Please feel free to reopen or open a new issue when you have more data.

[mrikh]

So I finally got about to trying to find a solution to this problem and ran into the following post here.

It talks of a workaround that does seem to work and is endorsed by apple engineers!

So what I did was at the end of:

- (instancetype)initWithConfiguration:(AWSServiceConfiguration *)serviceConfiguration
         transferUtilityConfiguration:(AWSS3TransferUtilityConfiguration *)transferUtilityConfiguration
                           identifier:(NSString *)identifier
                    completionHandler: (void (^)(NSError *_Nullable error)) completionHandler{

     //some insane work
     ....

     [[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(handleActive) name:UIApplicationDidBecomeActiveNotification object:nil];
     return self
}

I added a notification observer and then:

-(void) handleActive{
    if (@available(iOS 9.0, *)) {
        [_session getAllTasksWithCompletionHandler:^(NSArray<__kindof NSURLSessionTask *> * _Nonnull tasks) {
            [tasks enumerateObjectsUsingBlock:^(__kindof NSURLSessionTask * _Nonnull obj, NSUInteger idx, BOOL * _Nonnull stop) {
                if (obj.state == NSURLSessionTaskStateRunning){
                    [obj resume];
                }
            }];
        }];
    }
}

This works as expected. 0 problems.

[palpatim]

@mrikh Thanks for investigating and for bringing a solution to this old issue! The workaround seems reasonable, but I'm not inclined to build lifecycle monitoring into the TransferUtility, since we are already relying on customer apps to manage the lifecycle by recreating a TransferUtility with the appropriate key. In fact, the initialization code you reference above is the first part of that--it's invoked by a customer app creating a transfer utility with a key, as noted here.

The good news is, TransferUtility already loads tasks from the NSURLSession during its initialization! Although, it uses getAllWithCompletionHandler rather than the method specified in the workaround, getAllTasksWithCompletionHandler

So there's a small question of whether there something magic about invoking getAllTasksWithCompletionHandler, or whether the real magic that the workaround specifies to call resume on each running task?

It sounds like you've got a working test with a forked copy of the repo--would you be able to change the code in AWSS3TransferUtility.m around line 634 and test? We can certainly look at this, but it's not super high on our priority list, and it'll be a while before we're able to spin up a test to try to reproduce this and see if the code snippet below fixes it.

//If it is in any other status than running, then we need to recover by retrying.
if ([task state] != NSURLSessionTaskStateRunning) {
    //We think the task in IN_PROGRESS. The underlying task is not running.
    //Recover the situation by retrying.
    [self retryUpload:uploadTask];
    continue;
    // #################
    // ADD THIS ELSE CLAUSE
} else {
    [task resume];
    // #################
}

That will take care of testing this for upload tasks. If it works, we'll apply that patch to the section for download tasks as well down on line 680 or so.

Thanks again for the investigation!

[mrikh]

I understand. So the magic is with resume and not getAllTasks. It needs to be explicitly called when app comes back into the foreground. The app doesn't get suspended in this case, it is simply in the background. Unless of course there is a method inside the SDK that explicitly gets a callback when app comes back into an active state from a background state I doubt it'll work. The method in the lines you mention is linkTransfersToNSURLSession which doesn't get called when app comes back into the foreground. (Haven't really setup for upload tests)

I also tried

AWSS3TransferUtility.default().getDownloadTasks().result?.forEach({ (task) in
            (task as? AWSS3TransferUtilityTask)?.resume()
        })

To control it from the client side without the need to include the notification observer in the transfer utility but unfortunately that doesn't work. The reason I had to go inside to the TransferUtility class was so that I can find a reference to the _session object and manually iterate over the tasks!

[palpatim]

I also tried...

It looks like invoking resume on an AWSS3TransferUtilityTask checks that the TransferUtility task is paused before it invokes resume on the URLSession task, so I can see how that wouldn't work. You could try:

(task as? AWSS3TransferUtilityTask)?.sessionTask.resume()

to bypass that check and directly operate on the session's task. However, that's a last resort--we really want to handle this for customer apps, not make y'all do low-level operations on NSURLSession.

The app doesn't get suspended in this case, it is simply in the background. Unless of course there is a method inside the SDK that explicitly gets a callback when app comes back into an active state from a background state I doubt it'll work.

Let's think about this a bit more. Your point about the resume path is valid and I'm sorry I missed it. Our current instructions imply that TransferUtility should begin delivering progress events and callbacks as soon as the customer app attaches to them, without any additional method calls or wiring.

Further, it seems innocuous to call -[NSURLSessionTask resume] multiple times; the docs specify that it "Resumes the task, if it is suspended", which means to me that invoking resume multiple times on a task should be safe.

So we just have to 1) Listen to the correct lifecycle event; and 2) make sure that we resume the correct items.

For 1): your proposal to use a notification is a good one, but I think we want to listen to UIApplicationWillEnterForegroundNotification rather than UIApplicationDidBecomeActiveNotification. My read of the discussion in the Apple forum is that progress events are tied to background/foreground transitions, not active/inactive transitions. Active/inactive is quite a bit more chatty, since that transition may be invoked for control center swipes, app switcher launches, incoming calls, Siri notifications, etc, that do not actually transition the app to the background.

For 2): Rather than iterate over the session tasks directly, we should iterate over the TransferUtility tasks, inspect to make sure they are in AWSS3TransferUtilityTransferStatusInProgress, then invoke [sessionTask resume].

What do you think?

[mrikh]

Interestingly enough everything works fine on the simulator but not on device.

Well i never checked the instructions before. But i tried them with a print statement in applicationDidBecomeActive and willEnterForeground in AppDelegate just to make sure I didn't miss out on anything but it still didn't work.

1. There will be a lot of callbacks received no doubt. But in my tests I found that UIApplicationWillEnterForegroundNotification doesn't execute the code every time. Works probably 2/10 times. And UIApplicationDidBecomeActiveNotification works 100% of the time. I assume it is the will and did that screw it up, probably an internal race condition.

2. I am sorry but do you mean something like

NSArray<AWSS3TransferUtilityTask *>* tasks = [self getDownloadTasks].result;

and iterating over the results after checking for AWSS3TransferUtilityTransferStatusInProgress.

Sure can do that but is there a specific reason for it since we do end up modifying the NSURLSession object directly anyways?

[palpatim]

Interestingly enough everything works fine on the simulator but not on device.

A couple of notes:

1. Don't forget that backgrounding behavior is quite persnickety; Apple's recommendation is always to test on device if possible.
2. Remember that depending on the app you're using, AppDelegate isn't always called in the lifecycle (e.g., Scene-based apps don't have the same lifecycle methods). However, iOS delivers lifecycle notifications at about the same time as the appropriate lifecycle event whether it's a scene-based or AppDelegate based app.
3. If you're not seeing UIApplicationWillEnterForegroundNotification firing, that's cause for some concern, but it's almost certainly in the app or surrounding code. I've not heard of any reports of unreliable delivery of NotificationCenter notifications, and they're part of lots of critical path app code. :)
4. Finally, recall that the app doesn't move to background immediately, but only after a little bit of time has passed. If your test consists of rapidly backgrounding and foregrounding the test app, the system may not have transitioned the app to 'background' state yet, which means that there would be no corresponding transition to foreground, which means we wouldn't see notifications fired. However, if you're saying that applicationWillEnterForeground: gets called without a notification getting fired, then that's a different story, and one that probably needs more investigation.

Sure can do that but is there a specific reason for it since we do end up modifying the NSURLSession object directly anyways?

We don't want to resume an NSURLSession task that the TransferUtility has suspended. A customer app may explicitly suspend an upload or download; it would be a mistake for us to then ignore that and invoke resume on the underlying session object anyway.

[mrikh]

Interestingly enough everything works fine on the simulator but not on device.

By this I mean that the original bug in the first post doesn't appear on the simulator at all. Only appears on device.

If you're not seeing UIApplicationWillEnterForegroundNotification firing, that's cause for some concern, but it's almost certainly in the app or surrounding code. I've not heard of any reports of unreliable delivery of NotificationCenter notifications, and they're part of lots of critical path app code. :)

Ohhh I didn't mean that the Notification isn't fired, sorry I should have been clearer. I re-read what I wrote and it can be read like that. I mean that the putting the code inside the notification selector doesn't work! It only works 2/10 times. The selector is called but putting [urlSession resume] inside the method doesn't work.

A customer app may explicitly suspend an upload or download; it would be a mistake for us to then ignore that and invoke resume on the underlying session object anyway.

Ah thats what I had the below check for

if (obj.state == NSURLSessionTaskStateRunning){
     [obj resume];
}

I just automatically assumed that the AWSS3TransferUtility state would be same as NSURLSession's state would be same but sure, I completely agree with what you said.