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

Token Feedback (protocol version2) + Blacklist xmlrpc methods #31

Closed
wants to merge 2 commits into from

Conversation

endyman
Copy link

@endyman endyman commented May 15, 2012

This is the more solid version of the token feedback patch.
It also includes XMLRPC methods for server and clients to fetch the blacklist (which will be cleared afterwards) in order to deprovision the tokens.

endyman and others added 2 commits March 9, 2012 11:51
Invalid APNS tokens i.e. sandbox tokens will cause Apple to drop the
APNS SSL connection. By using the extended protocol we can provide an
ID with each message and Apple will send a binary notice prior closing
the connection with the token causing the problem and a reason code.
Tokens are stored in a "dict buffer" for later lookup. If a token
causes a connection drop, the token will be added to a global black
list (per appID). Next time the token will be filtered out. This will
at least provide a level of "self-healing" - Next step would be to
expose the black list via XML-RPC so that the client can fetch it (like
the feedback service data)
@samuraisam
Copy link
Owner

It would be nice if the xmlrpc methods could be separated in the way I did it in this branch:

https://github.com/samuraisam/pyapns/blob/rest-service/pyapns/server.py#L471

and

https://github.com/samuraisam/pyapns/blob/rest-service/pyapns/server.py#L631

That way users can use either version as necessary and lessens the possibility of breaking existing installations.

@endyman
Copy link
Author

endyman commented May 15, 2012

I wasn't aware that you already have an enhanced protocol branch - even with REST - nice. I agree that adding additional methods instead of modifying existing ones is way less intrusive. From my perspective it may make sense to just close both requests and use the rest-servie branch as a basis to include the blacklisting code

@gregbayer
Copy link

@samuraisam / @endyman

I'm planning to use pyapns in production soon, but we need support for v2 of the protocol and blacklisting tokens on the server. I'd rather use what you've got than re-invent the wheel and add it on myself. What's the best branch or clone repo to start with to pull in these features as of now?

@samuraisam
Copy link
Owner

@gregbayer use rest-service branch. Barring the REST service implementation (which is incomplete), the version 2 of the protocol is ready, and I have been using it in production for over a year.

@gregbayer
Copy link

Thanks! Will try it out.

Is there any implementation of immediate blacklisting of bad tokens as @endyman suggested in his pull requests? Or have you found that the feedback service is fast enough for notifying you of bad tokens?

We are generally concerned about pushes not going through because of bad tokens and would love a way to immediately identify these and even retry the rest of the messages that did not go through in that block.

Resending the rest of the tokens that weren't bad from a failed request doesn't happen in the REST service branch yet, does it? Also, I noticed you have a new 'log_disconnections' flag. But it's not available via the client yet?

Thanks for your help.

@samuraisam
Copy link
Owner

The way I've been doing it is to just send one notification at a time using notify2.

The only case in which you will have a block fail due to bad tokens is if you are sending bulk notifications over the xmlrpc connection.

disconnection_log is a part of the rest-service branch. Call service.log(app_id) to get a list of disconnections, their time, and the reason for the disconnection. I personally have never used it in production however, and stick to using the feedback service, as it works fine for me.

You can poll the disconnection log to get a list of responses from apple that occurred at the time of a disconnection. It's helpful for debugging errors in your own programming (too large of a payload, etc):

The errors are:

0: No errors encountered
1: Processing error
2: Missing device token
3: Missing topic
4: Missing payload
5: Invalid token size
6: Invalid topic size
7: Invalid payload size
8: Invalid token
255: None (unknown)

In my python app I just log these to Sentry every now again, though almost never see any.

@gregbayer
Copy link

Cool. Do you have any scalability concerns regarding only sending one notification at a time using notify2? Have you tried to do any load testing of this approach?

I know Instagram had a good experience using pyapns on just one server, but I assumed they were sending notifications in batches - so I was doing that too. My intuition says that switching to one at a time might require multiple pyapns servers at Instagram scale. What do you think?

@samuraisam
Copy link
Owner

pyapns is a twisted xmlrpc service and would be the primary bottleneck (the connection to apple would not). That being said, twisted.web (which the xmlrpc server is built on) is known to have pretty decent performance characteristics and could probably handle upwards of 2500-5000req/s or more depending on your hardware.

I can't speak for instagram, but your code is just going to be simpler if you send one at a time, even if you find you need extra servers. Assuming you have some form of network monitoring, just keep an eye on latency to the pyapns server, and if that starts to go up, you might want to load balance between two or more.

Also you could run on pypy which would give you a ~2-5x speed improvement if it becomes an issue.

In my application I just push messages onto a queue that celery picks off and sends to pyapns. I've not experienced any bottleneck yet and have sent a LOT of notifications (high throughput) this way for several years.

@gregbayer
Copy link

thanks! Sounds reasonable. I'll give it a try.

@samuraisam
Copy link
Owner

Also, if you have a chance, it would be great to merge that into master. I am the only one I know to be running it in production and would like to merge it in once there is someone else also on board.

@troppoli
Copy link

I'm looking for a little clarification of the status of the branches. @samuraisam it sounds like you're running in production off of the rest-service branch, but it doesn't currently seem to compile. @endyman's blacklist_xmlrpc appears to be the only place to get a working v2 impl. Is this correct? Any chance of notify2 showing up in master?

@samuraisam
Copy link
Owner

Hey @troppoli I will try to get this in soon. The version I'm using in production at Lightt is the current master head. I have some time next week and I'll try to merge in this functionality.

@troppoli
Copy link

cool, thx

@gregbayer
Copy link

@troppoli Sorry for not sending a pull request sooner. Based on @samuraisam's suggestion, we have been running the rest-service branch at Pulse for a while. We had to make these changes: #39

I meant to try merging in into master, but never go around to it. Let me know if you have any questions.

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.

4 participants