-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
base: master
Are you sure you want to change the base?
Conversation
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.
Jesus, people still use this thing? |
@mattyohe +1 for the wtf. |
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 :) In any case, check the disclaimer at the bottom, I strongly encourage everyone to use AFNetworking. |
My idea to help encourage people to use AFNetworking is mark all public methods as depreciated. i.e.,
Just a thought. :) |
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:
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. |
@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:
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.
Not sure how a 3rd-party
Not true. For individual operations, there's a download progress block, and for multiple operations, an HTTP client can enqueue a batch of operations.
ASI was correct in using CFNetwork when it was originally developed many years ago, when 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. |
@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. |
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.