aws-amplify / aws-sdk-ios

AWS SDK for iOS. For more information, see our web site:
https://aws-amplify.github.io/docs
Other
1.68k stars 884 forks source link

Best practice for pausing upload in AWSS3TransferManger and AWSS3TransferUtility on network loss? #769

Closed jeffjvick closed 5 years ago

jeffjvick commented 6 years ago

I start a large upload with TransferManager and part way through I turn on airplane mode to kill the wifi connection. This gives a "The Internet connection appears to be offline." error as expected since it does not appear that TransferManager handles network drops on its own like TransferUtility does.

To combat this I'm trying to catch the network loss myself and pause the uploads before TransferManager errors. I'm using the Reachability cocoapod https://github.com/ashleymills/Reachability.swift to detect network loss and it seems like it's a race to see if I can pause before TransferManager errors. Usually I lose.

I have not seen any official sdk documentation on how to handle pausing on a network loss so I wanted to see if someone could point to another source. Or possibly I should be handling this in a different manner?

Also, please don't tell me to use TransferUtility because TransferManger is "old". TransferUtility does not support multipart uploads so whenever the network is lost it just restarts from the beginning and my app will be uploading large (100+ mb) files on less than ideal networks and I can't force users to keep restarting. I don't understand why multi-part wasn't a priority for the new code base but that's the way it is so it appears I'm stuck with the old TransferManager.

Thanks

scb01 commented 6 years ago

@jeffjvick

Could you check in your scheduler if this is possibly what was happening? (new parts getting started in the background have the discretionary flag set).

Very nice analysis! I agree with your findings. This is exactly what is happening - The discretionary flag is set per session and defaults to No. In my code, I am using the default value when the NSURLSession is created. For transfers initiated with the app in the foreground, the system is using the value of No ; for transfers initiated with the app in the background, the OS is overriding what was set in the NSURLSession and treats it as discretionary, and therefore they progress slower. Let me look into this and see if I can find a way around it.

I will investigate the over-reporting issue and post back here in a few days.

scb01 commented 6 years ago

@jeffjvick

Here is an idea that may help get around this - there is a property in the AWSS3TransferUtilityConfiguration called multiPartConcurrencyLimit. This sets an upper limit in the number of parts that are being uploaded at any given point of time.

Can you adjust that up to accommodate your typical file sizes - if your typical filesize is 50 MB or lesser, then adjust it to at least 10 so that all the parts will be uploaded in parallel. It should result in each part being initiated when the app is in the foreground and speed things up. We don't need to worry about overloading the device as the background NSURLSession prioritization mechanism will kick in and only allow a subset of transfers to proceed at any given point.

scb01 commented 5 years ago

@jeffjvick Just a quick update - I ran some experiments over the weekend. The speeds are definitely much improved with a higher setting for the multiPartConcurrencyLimit. Let me know how it goes on your side once you've had a chance to try it out.

jeffjvick commented 5 years ago

@cbommas

I was able to try increasing the multiPartConcurrencyLimit. I had considered this but I thought I read somewhere that this had a max limit of 5. Anyway, for our use case the uploads are in the 100's of MB so we would need a very large limit to get everything initiated in the foreground. I tried setting it to 999 and tried some files around 200 MB and it seemed to work well (as in, not slowing down in the background) but this seems like "cheating". I wonder if iOS's prioritization may start to throttle the app's background transfers if it detects us not using discretionary on too many background transfers. I need to do more testing.

Are you aware of any max number on this limit? Is the 5 MB per part set in stone or can that be adjusted as well?

For another data point, I have tested Google's drive app and it does background uploads rapidly and appears to support multi-part. I'm not sure what they are doing under the hood to accommodate this.

jeffjvick commented 5 years ago

@cbommas

Hi, also I have another follow-on question for you. Why did you implement a background scheduler if it seems you can just create all the NSURLSession parts at once and let it do the scheduling? I assume to limit the number of concurrent parts uploading but it seems even in the foreground there is some sort of internal limit placed on this.

Jane930525 commented 5 years ago

@jeffjvick

Regarding - "One thing I noticed is that AWSS3TransferUtilityMultiPartUploadTask does not have a taskIdentifier while AWSS3TransferUtilityUploadTask does"

The Mulitpart transfer mechanism uses a NSURLSessionTask per part to upload the file in pieces. That is the reason why there is no taskIdentifier on it. All tasks, however, have a unique TransferID, that you should use instead of taskIdentifier to keep track of transfers.

Thank you for the detailed explanation.

scb01 commented 5 years ago

@jeffjvick

Why did you implement a background scheduler if it seems you can just create all the NSURLSession parts at once and let it do the scheduling?

The thinking essentially was to provide the controls to the developer on how "rich" of a mix they wanted to run using the multiPartConcurrencyLimit to tune. The one thing that did not occur to me was the impact of the OS prioritization logic that automatically applied the discretionary flag to the parts that were initiated when the app was in the background.

One of the ideas I am working on is to create all the NSURLSession tasks upfront, but only start up to the multiPartConcurrencyLimit. I will let you know in a few days how this goes.

rromanchuk commented 5 years ago

Are you any of you folks having issues with keeping your credentials alive? I'm getting some failed uploads with "The operation couldn’t be completed. (NSURLErrorDomain error -1001.)" I don't have a lot of insight yet, it sure feels like expired cognito credentials though, it looks like it happens when a user brings the app back up to the foreground after after some period of time.

I also feel like i'm abusing this with a very lazy singleton i'm using

 lazy var transferUtility: AWSS3TransferUtility = {
-        AWSS3TransferUtility.s3TransferUtility(forKey: "my-upload-manager")
 }()

This is probably abusive type of use, does anyone have any intimate knowledge of how transfer utility and cognito lifecycle works? When does the countdown begin, and is it smart enough to refresh on the next retry? I could see a number of ways this type of singleton abuse https://gist.github.com/rromanchuk/ca7b994be0fcf69387c8c6c74f7c36fb could be burning me

minbi commented 5 years ago

Hi @jeffjvick @Jane930525 @rromanchuk ,

We have just released version 2.8.2 of the SDK. Please check if this fixes the issue for you.

scb01 commented 5 years ago

@jeffjvick

This version has the change to create all the NSURLSession tasks upfront. It also has a new approach for calculating progress for multipart transfers - which should resolve the progress tracking issue that you have been encountering. Please upgrade to this when you have some time and let me know if you run into issues.

jeffjvick commented 5 years ago

@cbommas Hi, thanks, I will try testing it this evening.

stale[bot] commented 5 years ago

This issue has been automatically closed because of inactivity. Please open a new issue if are still encountering problems.

jeffjvick commented 5 years ago

@cbommas, Hi, so when I said I will test this in the "evening" I apparently meant 4 months from now... ;)

I am testing this using version 2.9.7 although I believe not much has changed regarding AWSS3 since 2.8.2. I am on an iOS 12.2 device.

I'm still have some issues after upgrading. I can open a new issue if you'd like but I'll just list them here for now:

1) Regarding the progressBlock, I have noticed that if while you are uploading you disable the network and then reenable it (causing the currently upload parts to restart) the progressBlock stops getting updates until the progress has surpassed the point where it left off. This is especially evident if you do a non-multipart upload. So for example you are uploading a 50 mb file and at 30% you turn on/of the network, it will restart the upload but the progress will just sit at 30% until the restarted upload passes back over 30%.

I looked into the AWS3TransferUtility.m code and see the issue. It is only updating transferUtilityUploadTask.progress.completedUnitCount if it is less than'totalBytesSent but this doesn't take into account if totalBytesSent gets reset when parts (or the whole) upload needs to get restarted. I think the easiest way to fix this would be to just use !=. I'm not sure why you have <. I made these two changes and it is working:

if (transferUtilityUploadTask.progress.completedUnitCount != totalBytesSent) {
    transferUtilityUploadTask.progress.completedUnitCount = totalBytesSent;

    if (transferUtilityUploadTask.expression.progressBlock) {
        transferUtilityUploadTask.expression.progressBlock(transferUtilityUploadTask, transferUtilityUploadTask.progress);
    }
}
if (transferUtilityMultiPartUploadTask.progress.completedUnitCount != totalSentSoFar ) {
    transferUtilityMultiPartUploadTask.progress.totalUnitCount = [transferUtilityMultiPartUploadTask.contentLength longLongValue];
    transferUtilityMultiPartUploadTask.progress.completedUnitCount = totalSentSoFar;

    //execute the callback to the progressblock if present.
    if (transferUtilityMultiPartUploadTask.expression.progressBlock) {
        AWSDDLogDebug(@"Total %lld, ProgressSoFar %lld", transferUtilityMultiPartUploadTask.progress.totalUnitCount, transferUtilityMultiPartUploadTask.progress.completedUnitCount);
        transferUtilityMultiPartUploadTask.expression.progressBlock(transferUtilityMultiPartUploadTask, transferUtilityMultiPartUploadTask.progress);
    }
}

2) Also regarding the progressBlock, I'm seeing now where if you start an upload and go the background (hit home button) and come back the progress reporting has stopped. It is still uploading though because if you wait a bit the upload will complete (completionHandlers will fire and progressBlock will get called once with a fractionComplete of 1.0). Sometimes if you go in and out of the background a few times you can get the progressBlock to start working again. Have you seen this behavior?

I haven't been able to test the new changes you made for multipart yet because I'm stuck on these issues. I'm not sure how someone didn't notice these problems with the progress reporting.

palpatim commented 5 years ago

@jeffjvick

Please do open a new issue with the repro steps and details of your setup so we can investigate.

jeffjvick commented 5 years ago

@palpatim @cbommas

I have made https://github.com/aws-amplify/aws-sdk-ios/issues/1512 and https://github.com/aws-amplify/aws-sdk-ios/issues/1513

jeffjvick commented 5 years ago

@palpatim @kvasukib

Hi, does someone have an update for #1512 or #1513 ?

It's been over one and half years since we tried to integrate TransferUtility into our project and it's still too broken for us to release to production. Our app is still using TransferManager.

navasiloy commented 4 years ago

Hi @jeffjvick

Looks like you did a great job finding all these small details! I've got the same task (100Mb upload). Can you tell, please, what settings you ended with for

Thank you in advance!

Guolanlan commented 4 years ago

I encountered a similar problem. After the app is killed and restarted, the upload will not resume, because the status of the fragmentation task has changed to error. I debugged it in the source code. When creating a temporary subtask, the path of my fragmentation task Not found, -(NSString ) createTemporaryFileForPart: (NSString ) fileName partNumber: (long) partNumber dataLength: (NSUInteger) dataLength error: (NSError **) error{

 //Check if the file exists.
 if (![[NSFileManager defaultManager] fileExistsAtPath:fileName]) {
     NSString *errorMessage = [NSString stringWithFormat:@"Local file not found. Unable to process Part #: %ld", partNumber];
     NSDictionary *userInfo = [NSDictionary dictionaryWithObject:errorMessage
                                                          forKey:@"Message"];
     if (error) {

and But I still don’t understand, why can’t I find this path? If uploadDataUsingMultiPart is used, this problem will not exist? ?