ADNKit
ADNKit copied to clipboard
joeldev
User Streams API Support
This technically works, but needs to be reviewed. Line 148 specifically. After the pause/setValue/resume exchange, the queue behaves strangely for at least one operation (not starting it.)
This can be merged now, per-approval (including the hacky fix on line 148.) I'd prefer said hack fixed before it gets merged though, if it's possible.
What's the hack, @kolinkrewinkel? I'm about to start an app build that needs user streams, so I'd be happy to have a look.
Sorry @kolinkrewinkel, I think I'm going to have to reject this for a few reasons. Firstly, the ShocketShuttle submodule should be added using the https repo URL instead of the SSH URL. Secondly, the project doesn't build the OS X framework target successfully, which is a deal-killer. Can you try taking another look at these issues? Once these issues are resolved I'd be happy to approve it. Thanks!
I'm going to see where I get this afternoon — I had a few hours to start a new app idea, but I can't start without OS X and user streams support. I'll submit a new PR referencing this one against the user-streams branch when it's ready.
:), thanks @joeldev. @tonyarnold, I may have resolved it, but it requires further review. It was with operations in the NSOperationQueue (-operationQueue) not starting after pausing the streaming operation, even after it was resumed. Querying it would reveal that they were simply lying there with the state "isReady." Recently it hasn't been giving me trouble, but it was such a weird quirk I'm sure it's something obvious I'm just not seeing.
A new set of issues is with SocketShuttle and SocketRocket. We need to roll our own forks (we already have one for SocketShuttle) for two reasons. One is that when the SocketShuttle is recreated, it appears to leave the SocketRocket instance alive for a time (or indefinitely as a leak—see point 2) which causes a message to be sent to a deallocated SocketShuttle instance. ARC would fix this just as a start (with weak references). Two is that just from the fact that the SRWebSocket instance isn't deallocating immediately, something is up with it internally (either it's keeping itself alive with retain cycles or failing to close, which I believe is a prerequisite to it deallocating.)
Also @tonyarnold, feel free to ping me with a private message (on ADN) and we can talk real-time and collaborate on it, if you want. Also available on AIM as kkrewink.
If you're planning on forking SocketRocket, check my fork for a couple of really minor project fixes.
Any plans to fork/merge @tonyarnold's changes for SocketRocket? I had a to make a few changes to get it to build debug on 10.8. I also have a few changes to ADNKit and SocketShuttle to get OS X working that I can get a PR put together for.
We don't actually have a repository for that (and that's not my area.) I'll work on it more when I have time... first day of sophomore year went from "free coding day" to "don't show me Objective-C; I need sleep."
No problem. I'll commit my changes to a fork and sort out a pull request for @joeldev to consider.
Any word on when this will be merged into master?
I can confirm this works well on OS X (using @tternes fork, with one minor change in SocketShuttle to get the dependencies to install).
What do you guys think about getting this merged to master?
@kolinkrewinkel, @mthurman and I hacked around on this last weekend (well, really just those two guys), and it seems to work well. I'm going to try to bring this up to date so that it's based on top of master, and then we can see how it goes.
As soon as it's brought up to date, it can be merged.
Sent from my iPad
On Jan 30, 2014, at 17:32, Bryan Berg [email protected] wrote:
What do you guys think about getting this merged to master?
@kolinkrewinkel, @mthurman and I hacked around on this last weekend (well, really just those two guys), and it seems to work well. I'm going to try to bring this up to date so that it's based on top of master, and then we can see how it goes.
— Reply to this email directly or view it on GitHub.
Does anyone know how we can go about adding a dependency to joeldev/SocketShuttle in the pod spec? I'm not well versed in pod-knowledge and AFAIK you can't do git references within a pod spec. It's the only hold back before merging.
You'd have to create a new podspec and submit it — something like ADNKitSocketShuttle. What's holding the changes back from being merged upstream?
We've never actually talked about merging them back up. I guess there's nothing wrong with doing so, though.
I dunno, it just seems like good form to offer to push them back upstream and then use that as your dependency.
Good call, @tonyarnold. My first step is going to be to take all of the substantive changes from the SocketShuttle fork, make them backwards compatible, and (try to) get them accepted upstream.
OK, sent in a pull request for SocketShuttle as a start: https://github.com/mk/SocketShuttle/pull/3
It's basically all of the meat of Kolin's SocketShuttle patches, without the packaging changes, which I think we can accommodate inside of the ADNKit project without requiring a custom fork of SocketShuttle. This should make it easier to keep the projects manageable both by CocoaPods and git submodule users.
One thing I did run into here, which @kolinkrewinkel might have already addressed, but I haven't yet found: in my code I copy ANKClient a lot, specifically because I want to make requests with the same access token but with different pagination parameters. We should probably ensure that this doesn't mean we're creating new sockets every time (I think it's a fairly common pattern.)
Couple things: firstly, thanks. My initial inclination is towards a singleton socket, or—even better—one that sticks per authentication token.
Now that we're in the conclusion phase, I'm curious what my rationale was to destroying and recreating the socket shuttle instances on disconnect or error. In my opinion this defeats the purpose of using SocketShuttle over SocketRocket (among other things.) So, before merging, ideally, I'd like to investigate...not doing that.
Sent from my iPhone
On Feb 17, 2014, at 10:42, Bryan Berg [email protected] wrote:
OK, sent in a pull request for SocketShuttle as a start: mk/SocketShuttle#3
It's basically all of the meat of Kolin's SocketShuttle patches, without the packaging changes, which I think we can accommodate inside of the ADNKit project without requiring a custom fork of SocketShuttle. This should make it easier to keep the projects manageable both by CocoaPods and git submodule users.
One thing I did run into here, which @kolinkrewinkel might have already addressed, but I haven't yet found: in my code I copy ANKClient a lot, specifically because I want to make requests with the same access token but with different pagination parameters. We should probably ensure that this doesn't mean we're creating new sockets every time (I think it's a fairly common pattern.)
— Reply to this email directly or view it on GitHub.
Yeah, one per token would probably be best — multi-account support would be a bit hard with a singleton. Maybe a singleton socket handler that vends socket instances per client? Would you ever need to establish more than one socket per app per client?
Not that I can think of. Even changing pagination settings wouldn't warrant creating a new socket as it's only for the short-lifetime of the request.
I was just going to suggest creating a singleton containing a weak NSHashMap of access_token -> SocketShuttle instances and having the ANKClient lazily create (and retain) them when needed, nilling the reference when the accessToken property changes.
Has this been brought up to date as per @berg's earlier comments? I'm debating dropping this in in place of my own, manual AFNetworking/Socket-based code in a project.
Update: it might be up-to-date, but it doesn't compile out of the box on OS X. SocketRocket/SocketShuttle keep trying to build the iOS static libs :cry:
So, remaining, (and feel free to suggest, so I can add to this):
- [ ] Test and hopefully amend destruction and recreation of
socketShuttleproperty. - [ ] Change to global per-token pattern of sockets, rather than per-client.
- [ ] Have SocketShuttle PR merged.
- [ ] Test fresh installs with both submodules and CocoaPods.
- [ ] Documentation
- [ ] Merge! ✨
Silly question, but how do I subscribe to a user's stream? Maybe add:
- [ ] Documentation
Update: Sorted this out privately with @kolinkrewinkel — I was on the right track, but misunderstood ADN's recommendations when starting a stream