New Progress Reporting API using `NSProgress` #3187

kcharwood · 2015-12-01T15:55:56Z

These changes expose a simpler, block based progress reporting API, while maintaining the ability for more advanced features using KVO on NSProgress.

Issues #3158 and #3163 got me thinking that progress was easier to use with old NSURLConnection based API's, and we shouldn't force anyone to use KVO for progress updating, so I decided to take a stab at implementing a block based approach, while also streamlining the accessors for individual upload and download progress.

Note that NSURLSessionTask sees progress as two distinct items: upload progress and download progress, as reported by countOfBytesExpectedToReceive, countOfBytesReceived, countOfBytesExpectedToSend, and countOfBytesSent. Therefore, I've decided to track these two things independently with two distinct progress objects, managed by the internal AFURLSessionManagerTaskDelegate.

API Changes

Additional convenience API's in AFHTTPSessionManager exposing download progress block for GET, and upload progress block for POST.
Streamlined API in AFURLSessionManager allowing upload and/or download NSProgress objects to be retrieved for any task.
Additional convenience API's in AFURLSessionManager exposing download and upload progress blocks for data task, download progress block for download tasks, and upload progress block for upload tasks.

Open Questions

Should the methods not containing the progress blocks in AFHTTPSessionManager be deprecated and/or removed? For example, there are now four POST methods in that API, rather than two as before.
- I am leaning towards removing them completely and listing it as an upgrade note.
Currently, the progress blocks are called on the session queue, not the main queue. I tinkered with dispatching them to the main queue, but it resulted in "duplicated" progress reporting because the progress object was updated again on the session queue before the main queue was called, making it seem like there actually wasn't a change in progress. Should we care about this?
- Progress Updated to 2%. Dispatched to main queue
- Progress Updated to 5%. Dispatched to main queue
- Dispatch called on main queue. Reports 5% progress since its a reference to the current progress
- Dispatch called on main queue. Reports 5% progress since its a reference to the current progress

@0xced @cnoon @jshier @kylef would love your eyes on this. Probably the biggest code change so far in the 3.0.0 branch. I think I got all the KVO management setup internally correctly, but would appreicate a review.

kcharwood · 2015-12-01T15:58:15Z

AFNetworking/AFHTTPSessionManager.h

+                              progress:(nullable void (^)(NSProgress *downloadProgress)) downloadProgress
+                               success:(nullable void (^)(NSURLSessionDataTask *task, id _Nullable responseObject))success
+                               failure:(nullable void (^)(NSURLSessionDataTask * __nullable task, NSError *error))failure;
+


Should the sibling method be deprecated?

kcharwood · 2015-12-01T16:16:15Z

Also note this does not implicitly add progress to the current progress. The user would need to do this explicitly.

cnoon · 2015-12-02T21:41:01Z

Discussed this issue in-depth on Slack with @kcharwood and I think this is the correct approach. Passing the NSProgress back through the progress block on the session queue seems like the best overall solution. 👍🏼

kcharwood · 2015-12-02T21:47:10Z

And lines up with the queue that Alamofire passes back the progress block.

kcharwood · 2015-12-03T22:27:18Z

I went ahead and added a deprecated flag to the older methods in the API, and will release beta 3 with them there.

We can then decide to pull them for the final release, or leave them in to remove for a future release.

@0xced @jshier @kylef I'd like to get this merged in first thing in the morning, and a new release cut tomorrow. Let me know if you have any other feedback!

kcharwood · 2015-12-04T16:03:04Z

Once I merge this in today, I'd still like to get some more feedback from the community as we wrap up the beta period. All thoughts are welcome!

These changes expose a simpler, block based progress reporting API, while maintaining the ability for more advanced features using KVO on NSProgress.

codecov-io · 2015-12-04T18:41:19Z

Current coverage is `66.22%`

Merging #3187 into 3_0_0_branch will increase coverage by +11.90% as of d0e5958

Powered by Codecov. Updated on successful CI builds.

New Progress Reporting API using `NSProgress`

aozhimin · 2016-01-14T05:23:22Z

@kcharwood , In AFURLSessionManager KVO For Progress ,the invoke sequence is different in iOS 8.x
and 9.x. in iOS8.x is like this:
2016-01-14 13:21:41.592 Durandal_Example[11933:139499] keypath:countOfBytesSent
2016-01-14 13:21:41.593 Durandal_Example[11933:139499] keypath:fractionCompleted
2016-01-14 13:21:41.593 Durandal_Example[11933:139499] progress = 1.000000
2016-01-14 13:21:41.593 Durandal_Example[11933:139499] keypath:countOfBytesExpectedToSend
2016-01-14 13:21:41.594 Durandal_Example[11933:139499] keypath:fractionCompleted
2016-01-14 13:21:41.594 Durandal_Example[11933:139499] progress = 0.143955
2016-01-14 13:21:41.594 Durandal_Example[11933:139499] keypath:countOfBytesSent
2016-01-14 13:21:41.595 Durandal_Example[11933:139499] keypath:fractionCompleted
2016-01-14 13:21:41.595 Durandal_Example[11933:139499] progress = 0.287910

and in iOS9.x is Like this:
2016-01-14 13:11:01.015 Durandal_Example[11732:133169] keypath:countOfBytesSent
2016-01-14 13:11:01.015 Durandal_Example[11732:133169] keypath:countOfBytesExpectedToSend
2016-01-14 13:11:01.016 Durandal_Example[11732:133169] keypath:fractionCompleted
2016-01-14 13:11:01.017 Durandal_Example[11732:133169] progress = 0.143955
2016-01-14 13:11:01.018 Durandal_Example[11732:133169] keypath:countOfBytesSent

as you can see in iOS 8 i will get fractionCompleted 1.000000 , that's incorrect, cause this KVO invoke before countOfBytesExpectedToSend invoke. so countOfBytesExpectedToSend is equal to 0.

fulldecent · 2016-01-17T06:50:24Z

This issue is the documentation for v3.0 API changes per https://github.com/AFNetworking/AFNetworking/wiki/AFNetworking-3.0-Migration-Guide

Just as a note to future readers, in AFN 2.0, your PUT request progress could be tracked in a block like this:

    request.setUploadProgressBlock({(bytesWritten: UInt, totalBytesWritten: Int64, totalBytesExpectedToWrite: Int64) -> Void in
        if totalBytesExpectedToWrite > 0 {
            progressBlock?(progress: Float(totalBytesWritten) / Float(totalBytesExpectedToWrite))
        }
    })

However, in AFN 3.0, the only option available is to use KVO on the NSProgress like this:

        let progress = requestManager.uploadProgressForTask(request)

and then you need to set up observers.

Perhaps this was not intentional, and PUT may get more love in the future.

aozhimin · 2016-01-18T02:52:08Z

@fulldecent , you can read AF3.0 code ,in AF, it has use KVO to nofity progress changes.

The impetus for this change was a bug related to: https://github.com/AFNetworking/AFNetworking/issues/3380 Which points to an issue where AFURLSessionManager could get in a state where it does not inform the AFURLSessionManagerTaskDelegate to un-observe cancelled (or otherwise completed) tasks before they are deallocated. While the above issue is specific to background tasks/sessions, we have observed it on sessions with default configurations as well. Instead of debugging the KVO issues, I decided to replace the KVO code with in favor of the appropriate NSURLSession delegate methods. Running all unit tests shows that this approach appears to be working. The pull request that made this change (AFNetworking#3187) mentions that its goal was to remove the need for AFNetworking API consumers to use a KVO API for getting progress. This makes sense, but I was unable to see any reason why KVO was used internally for getting progress information rather than the NSURLSession Delegate methods. Because of this, It isn't clear to me what the historical reasons were/are for KVO being needed to satisfy the progress requirements of the AFNetworking API. With any luck, KVO is no longer needed and we can eliminate this class of crashes by eliminating KVO.

kcharwood added the progress reporting label Dec 1, 2015

kcharwood added this to the 3.0.0 milestone Dec 1, 2015

kcharwood reviewed Dec 1, 2015
View reviewed changes

kcharwood mentioned this pull request Dec 1, 2015

add POST api with progress #3158

Closed

kcharwood added the changed label Dec 1, 2015

kcharwood changed the title ~~New Progress Reporting API~~ New Progress Reporting API using NSProgress Dec 4, 2015

kcharwood added 4 commits December 4, 2015 11:18

New Progress Reporting API

210c260

These changes expose a simpler, block based progress reporting API, while maintaining the ability for more advanced features using KVO on NSProgress.

Fixed issue where task was being captured in a block

86ade4e

Added a deprecated flag to older methods in the API

2adeda5

Changed __nullable to _Nullable

423d83e

kcharwood force-pushed the feature/progress_improvement branch from 0e98de4 to 423d83e Compare December 4, 2015 17:18

kcharwood added a commit that referenced this pull request Dec 4, 2015

Merge pull request #3187 from AFNetworking/feature/progress_improvement

3106e9d

New Progress Reporting API using `NSProgress`

kcharwood merged commit 3106e9d into 3_0_0_branch Dec 4, 2015

kcharwood deleted the feature/progress_improvement branch December 4, 2015 19:39

kcharwood mentioned this pull request Jan 11, 2016

Fixes #3271, Progress blocks not called on main thread #3272

Closed

kcharwood mentioned this pull request Jan 19, 2016

Tighten the description of the progress parameter #3299

Closed

coreyfloyd mentioned this pull request Jul 6, 2016

Remove KVO of progress in favor of using the NSURLSession delegate APIs wikimedia/AFNetworking#1

Merged

coreyfloyd mentioned this pull request Jul 6, 2016

Remove KVO of progress in favor of using the NSURLSession delegate APIs #3607

Merged

nicolelehrer mentioned this pull request Dec 13, 2016

VIM-4803: Download Crash vimeo/VimeoUpload#120

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

New Progress Reporting API using `NSProgress` #3187

New Progress Reporting API using `NSProgress` #3187

kcharwood commented Dec 1, 2015

Uh oh!

kcharwood Dec 1, 2015

Uh oh!

kcharwood commented Dec 1, 2015

Uh oh!

cnoon commented Dec 2, 2015

Uh oh!

kcharwood commented Dec 2, 2015

Uh oh!

kcharwood commented Dec 3, 2015

Uh oh!

kcharwood commented Dec 4, 2015

Uh oh!

codecov-io commented Dec 4, 2015

Uh oh!

aozhimin commented Jan 14, 2016

Uh oh!

fulldecent commented Jan 17, 2016

Uh oh!

aozhimin commented Jan 18, 2016

Uh oh!

New Progress Reporting API using NSProgress #3187

New Progress Reporting API using NSProgress #3187

Conversation

kcharwood commented Dec 1, 2015

API Changes

Open Questions

Uh oh!

kcharwood Dec 1, 2015

Choose a reason for hiding this comment

Uh oh!

kcharwood commented Dec 1, 2015

Uh oh!

cnoon commented Dec 2, 2015

Uh oh!

kcharwood commented Dec 2, 2015

Uh oh!

kcharwood commented Dec 3, 2015

Uh oh!

kcharwood commented Dec 4, 2015

Uh oh!

codecov-io commented Dec 4, 2015

Current coverage is 66.22%

Uh oh!

aozhimin commented Jan 14, 2016

Uh oh!

fulldecent commented Jan 17, 2016

Uh oh!

aozhimin commented Jan 18, 2016

Uh oh!

New Progress Reporting API using `NSProgress` #3187

New Progress Reporting API using `NSProgress` #3187

Current coverage is `66.22%`