Replace crashing KVO with manually method call #406
Conversation
| + (void)load { | ||
| /** | ||
| Swizzle NSURLSessionTask in AFNetworking's way | ||
| See AFURLSessionManager.m in AFNetworking project for detail |
There was a problem hiding this comment.
Do you mind adding a link to the issue: #390
| _startTime = 0; | ||
| _endTime = 0; | ||
| [task addObserver:self forKeyPath:kPINURLSessionTaskStateKey options:0 context:nil]; | ||
| + (void)load { |
There was a problem hiding this comment.
I wonder if there's a way to avoid using load? This will increase startup time which I like to avoid as much as possible. Could we possibly make this PIN_setupSessionTaskObserver and make it a class method with a dispatch_once inside? Then put all the code within a dispatch_once?
There was a problem hiding this comment.
I also prefer to avoid using of load, which is introduced for method swizzling. But I don't know how to update the PIN_startTime correctly instead of hooking resume method. In order to make method swizzling work correctly, I think load method cannot be avoid.
Do you think it's right if I add a new method call to update PIN_startTime manually after call of task's resume?
| #pragma clang diagnostic push | ||
| #pragma clang diagnostic ignored "-Wundeclared-selector" | ||
| IMP classResumeIMP = method_getImplementation(class_getInstanceMethod(currentClass, @selector(resume))); | ||
| Method afResumeMethod = class_getInstanceMethod(currentClass, @selector(af_resume)); |
There was a problem hiding this comment.
Looks like this code was copied from AFNetworking, I don't think their license allows that without also duplicating their license.
There was a problem hiding this comment.
Yeah, it is. I could change to a new style if method swizzling is still needed.
|
Thanks for the information on iOS 11's new session metrics features. I haven't learned about it yet. |
|
@garrettmoon which version will fix the crash? |
|
So I decided to abandon method swizzling, and update time properties manually. As for session metrics features, I haven't given time to learn more about it. Sorry for that. |
|
🚫 CI failed with log |
|
🚫 CI failed with log |
xilin
changed the title
|
About the failure test |
|
🚫 CI failed with log |
There was a problem hiding this comment.
I don't think this method will work because we can't guarantee a task will start when we request the system to start it. For example, if we configure the maximum number of connections to a given host to be 2 and then we resume 5 tasks, only 2 will start right away, but all 5 will be marked as started :(
| @@ -106,7 +105,7 @@ - (void)scheduleDownloadsIfNeeded | |||
| NSURLSessionDataTask *task = [queue firstObject]; | |||
| [queue removeObjectAtIndex:0]; | |||
| [task resume]; | |||
|
|
|||
| [task PIN_updateStartTime]; | |||
There was a problem hiding this comment.
So sadly, this isn't when the system will guarantee starting the task :( It's when we ask it to, but the system can choose to delay starting it…
There was a problem hiding this comment.
So method swizzling will not work neither, since it will update the start time immediately when resume is called.
|
@xilin that test only passes on slow network and is unreliable, sorry! Ignore that test and I'll make sure to rerun the CI until it passes before we merge. |
|
It seems that KVO is the best option for the feature currently. I'll close the PR and see if there is the other way to do it right. |
See the issue 390.
Same thing happened in AFNetworking (PR 2411 and 2702).
So I tried to do the same thing in PINRemoteImage using method swizzle instead of KVO.