★ wanayoo — archive 1999 https://github.com/AFNetworking/AFNetworking/pull/3718Nouvelle recherche | Portail wanayoo
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed potential KVO crasher for URL Session Task delegates #3718

Merged
merged 2 commits into from Oct 3, 2016

Conversation

@0xced
Copy link
Collaborator

@0xced 0xced commented Sep 30, 2016

Please see both commit comments for a full explanation.

0xced added 2 commits Sep 30, 2016
Adding self as observer in init and removing self as observer in dealloc is the best way to ensure a proper KVO implementation.
The dispatching on main thread was introduced in 920e266 in order to fix #2053. It was not a proper fix but rather a lucky coincidence. This kind of KVO error must be fixed by ensuring that observer are properly registered and unregistered, not by dispatching some random method on the main thread.

It is very important not to dispatch async session invalidation code because it is called in unit tests from the `tearDown` method. At that point, there’s no active runloop, so the session invalidation would not happen immediately but when running the next asynchronous unit test, i.e. when the `waitForExpectationsWithCommonTimeoutUsingHandler:` method is called.

This commit, in conjunction with d869571 fixes #3710.
@codecov-io
Copy link

@codecov-io codecov-io commented Sep 30, 2016

Current coverage is 87.81% (diff: 86.66%)

Merging #3718 into master will increase coverage by 0.08%

@@             master      #3718   diff @@
==========================================
  Files            44         44          
  Lines          6093       6064    -29   
  Methods        1084       1079     -5   
  Messages          0          0          
  Branches        406        406          
==========================================
- Hits           5345       5325    -20   
+ Misses          745        736     -9   
  Partials          3          3          

Powered by Codecov. Last update 4fb0518...2163a8b

@kcharwood
Copy link
Contributor

@kcharwood kcharwood commented Oct 3, 2016

This is great work @0xced! I was actually going down the same rabbit hole last week before my vacation, so I agree with all of your assessment here!

I'm verifying these changes locally as well right now. Will merge soon assuming I find no other issues.

🍻

@kcharwood kcharwood changed the title Fix potential KVO crasher Fixed potential KVO crasher for URL Session Task delegates Oct 3, 2016
@kcharwood kcharwood merged commit dd674a3 into AFNetworking:master Oct 3, 2016
2 of 3 checks passed
2 of 3 checks passed
codecov/patch 86.66% of diff hit (target 87.72%)
Details
codecov/project 87.81% (+0.08%) compared to 4fb0518
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kcharwood kcharwood added the fixed label Oct 5, 2016
@0xced 0xced deleted the 0xced:fix-potential-KVO-crasher branch Oct 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.