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

Fixing very obvious wrong behaviour when cleaning up the blocks that could cause many different-hard to figure out crashes #345

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JaviSoto
Copy link

There was a code added to make sure that the blocks were deallocated on the main thread, and it was wrong (it was over-releasing an NSMutableArray). Somebody fixed it by removing the second release, which is the wrong fix. There's an obvious code smell seeing how there's a performSelector: to a method that does nothing....
This is one of the thounsands of wrong things in ASIHTTPRequest, specially in memory management, I strongly recomment to all the folks out there that you migrate to AFNetworking, but I'm pushing the pull request in case somebody is getting crashes from this.

There was a code added to make sure that the blocks were deallocated on the main thread, and it was wrong (it was over-releasing an NSMutableArray). Somebody fixed it by removing the second release, which is the wrong fix. There's an obvious code smell seeing how there's a performSelector: to a method that does nothing....
This is one of the thounsands of wrong things in ASIHTTPRequest, specially in memory management, I strongly recomment to all the folks out there that you migrate to AFNetworking, but I'm pushing the pull request in case somebody is getting crashes from this.
@mattyohe
Copy link

mattyohe commented Nov 3, 2012

Jesus, people still use this thing?

@jeremytregunna
Copy link

@mattyohe +1 for the wtf.

@JaviSoto
Copy link
Author

JaviSoto commented Nov 3, 2012

Hahahahah yeah, I know. I don't use it, but I happened to have to fix a bug on an app that used to use ASI, but the newer version moved to AFNetworking, but the newer version wasn't out there yet :)
I found this and I thought it might be useful for someone...

In any case, check the disclaimer at the bottom, I strongly encourage everyone to use AFNetworking.

@jeremytregunna
Copy link

My idea to help encourage people to use AFNetworking is mark all public methods as depreciated. i.e.,

- (void)publicMethod __attribute__((deprecated));

Just a thought. :)

@smolls
Copy link

smolls commented Nov 15, 2012

Not to play devil's advocate here, but AFNetworking's decision to base their entire structure around NSURLConnection instead of CFNetwork inherently means it will be an immature framework compared to ASIHTTPRequest for the foreseeable future.

Reasons for still using ASI:

  • Support for all iOS 4.x users (still tons of armv6 users out there)
  • Proper caching without additional 3rd party hacks around NSURLConnection's incomplete implementation.
  • Built-in etag handling.
  • Any kind of authentication method beyond HTTP Basic Auth (proxy auth, as an example)
  • Non-arc codebases - ARC until very recently (Xcode 4.4) had some serious bugs associated with blocks and memory management. There are still outstanding issues related to ARC - go checkout the Clang/LLVM mailing lists & bug lists if you want to see some of the weird gymnastics the compiler does incorrectly :-)
  • ASINetworkQueue is far superior for tracking download progress over multiple operations. AFNetworking has no capabilities in this regard.

I could go into more detail, but really I think I'm just "feeding the trolls" here - there was a good reason ASI was built on top of CFNetwork vs. NSURLConnection. Yeah, there are some weird bugs here and there (thanks to the OP for the bug fix) but overall its a solid framework.

@mattt
Copy link

mattt commented Nov 15, 2012

@smolls I really don't want to have this discussion, but I can't help wanting to offer some corrections to your statement:

First, point by point:

  • Support for all iOS 4.x users (still tons of armv6 users out there)
  • Non-arc codebases - ARC until very recently (Xcode 4.4) had some serious bugs associated with blocks and memory management

AFNetworking 0.10.x supports iOS 4, and does not use ARC. FWIW, iOS 4 support should not be an issue for new apps, and FUD about ARC represents a pretty marginal position these days.

  • Proper caching without additional 3rd party hacks around NSURLConnection's incomplete implementation.

Not sure how a 3rd-party NSURLCache subclass qualifies as a "hack".

  • Built-in etag handling.

NSURLConnection handles this.

  • Any kind of authentication method beyond HTTP Basic Auth (proxy auth, as an example)

NSURLConnection supports much more than that, including proxy authentication. See the docs on authentication challenges

  • ASINetworkQueue is far superior for tracking download progress over multiple operations. AFNetworking has no capabilities in this regard.

Not true. For individual operations, there's a download progress block, and for multiple operations, an HTTP client can enqueue a batch of operations.

I could go into more detail, but really I think I'm just "feeding the trolls" here - there was a good reason ASI was built on top of CFNetwork vs. NSURLConnection. Yeah, there are some weird bugs here and there (thanks to the OP for the bug fix) but overall its a solid framework.

ASI was correct in using CFNetwork when it was originally developed many years ago, when NSURLConnection wasn't all that it is now. It served its purpose, and helped a lot of people out. I'm very thankful for the work everyone put into the library. But let's be honest—ASI really hasn't aged well.

There may well be some edge case features that AFN does not or will not implement, but for a large majority of people, a more modern networking library like AFN is almost certainly a better choice.

@smolls
Copy link

smolls commented Nov 15, 2012

@mattt Fair points, even though I think some of them are incorrect (SDURLCache is a bit of a hack, be honest. It's a fork of an Apple subclass that should not really be subclassed in the first place). I was unaware of the auth updates for NSRLConnection.

However, there's no FUD around the ARC issues - I've been dealing with them for the past week at my day job. Sure it's easy enough to enable ARC for just certain files, but as AFN is using __weak that's not possible for iOS 4 support in projects that need it. iOS 5+ support for new projects is valid, but ASI is still in use in a lot of major applications which can not just drop support. I haven't been keeping track of 0.10 vs 1.0 support, but I'm guessing there are bug fixes in 1.0 that aren't in 0.10.x of AFN.

Just to be clear, I'm not specifically trying to denounce AFN - It's a great project and I plan to move to it eventually for my projects. ASI would need core sections updated / rewritten to match what Apple and the community is doing these days, and really with AFN there it's not worth the effort. That being said, I'm still not sold on the block bandwagon that everyone is jumping on as a replacement for delegation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants