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.

Sign up for GitHub

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

Update Reachability.m #123

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+5 −3
Open

Update Reachability.m #123

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions Reachability.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,9 @@ +(instancetype)reachabilityWithHostname:(NSString*)hostname
SCNetworkReachabilityRef ref = SCNetworkReachabilityCreateWithName(NULL, [hostname UTF8String]);
if (ref)
{
id reachability = [[self alloc] initWithReachabilityRef:ref];

id reachability = [[self alloc] initWithReachabilityRef:CFBridgingRetain((__bridge id)ref)];
CFRelease(ref);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there, I saw your fix and wanted to leave a little feedback. I've been monitoring this issue closely I think you're the fourth person now I've interacted on a PR with :)

At line 98 retain count of the ref will be 1, due to the create.

We don't know immediately what the init does, so lets assume it doesn't retain it.

Now here on 102, you release ref. Its retain count is now 0. You will have created a zombie

(Final note) The init doesn't retain it, so all the above assumptions are true. Try and run the app with your changes, the reachability class will hit a zombie. See my solution #118 for the full fix. You are correct in that you whenever you create a foundational item you should release it. However what you, and a couple others, ran into was the assumption that the init takes ownership. Which it doesn't at the moment. If you update the init to take ownership then your fix will be complete. :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see what you did. I missed it and I assumed you did what others did before.

You can disregard my above comments.

However, I'd like to leave 2 cents if you don't mind :)

Not sure about everyone else but for me your fix is a little hard to read. I totally glanced over your retain statement. Since the ultimate (final) release is in the dealloc, I think many people would expect the retain to happen in the init. Instead of all the helpers that call the init. And while there may not be additional helpers being added any time soon, if someone does, they'll have to be sure to make the retain in their new helper. Since the current helpers are class methods, it does feel a little odd its doing memory management on an instance item.

Again, just my two cents & it may be a little nit-picky, but feels a little anti-pattern.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As much as the analyzer doesn't report possible problems, I don't really care what solution is bes among those two. They both look fine to me.
However don't blame others if you don't read code carefully... ;-)


return reachability;
}

Expand All @@ -111,7 +112,8 @@ +(instancetype)reachabilityWithAddress:(void *)hostAddress
SCNetworkReachabilityRef ref = SCNetworkReachabilityCreateWithAddress(kCFAllocatorDefault, (const struct sockaddr*)hostAddress);
if (ref)
{
id reachability = [[self alloc] initWithReachabilityRef:ref];
id reachability = [[self alloc] initWithReachabilityRef:CFBridgingRetain((__bridge id)ref)];
CFRelease(ref);

return reachability;
}
Expand Down