Remove KVO of progress in favor of using the NSURLSession delegate APIs #3607
Conversation
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 I'd like to get your thoughts on this PR since I saw you were the author of #3187 which added the KVO. Is it possible to shed some light on why the delegate methods were removed and replaced with KVO? I assume that either the APIs changed or some of the APIs were unreliable, but the unit tests didn't reveal any issues. Thanks for looking |
|
What is the status of this PR? Could we get one of the AFNetworking contributors (cc: @kcharwood ) to look over this because I am experiencing crashes caused by this problem. |
|
Sorry @coreyfloyd for just now looking. Been a busy summer with lots of travel in Swift 3 migrations every two weeks... I think I originally went the KVO route because of documentation I read somewhere, but I can't seem to find that at the moment... Looking at the docs now, Apple clearly says to use these delegate callback methods, so I think this is a good approach. I'll pull it down and run the unit tests to verify 👍 |
|
@kcharwood thanks for looking this over and getting this merged. Sorry I missed your comment earlier. I guess the tests passed for you locally? |
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 (#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.