Skip to content
This repository has been archived by the owner on Nov 9, 2018. It is now read-only.

Re-using or pooling connections with different notifications to avoid spamming the APN services #31

Open
mbargiel opened this issue Nov 1, 2013 · 10 comments

Comments

@mbargiel
Copy link
Collaborator

mbargiel commented Nov 1, 2013

We ran into a problem recently. Unless I'm mistaken, the typical usage of the APNService.push_notification_to_devices method seems to be oriented towards sending push notifications occasionally to a large number of devices.

However we're using this app to implement a real-time messaging system in our app, which means we're very very often sending notifications to select devices. Therefore, the current implementation constantly opens and closes SSL connections, which could be interpreted by the APN service as a DoS. Indeed, https://developer.apple.com/library/ios/technotes/tn2265/_index.html states:

"As is documented in the Local and Push Notification Programming Guide, developers are expected to open a connection and leave it open. If a connection is opened and closed repeatedly, APNs will treat it as a denial of service attack and block connections for a period of time."

I think a great feature (or even, a necessary feature, for this type of use case) would be to keep a connection cached in the model for each APNService instance ID, perhaps with a TTL to close if after a certain amount of time has passed without sending notifications, so that they can be re-used. Extra care would be necessary for the async/multi-threaded (or multi-process?) scenarios, such as when using Django celery.

What do you think?

(Note: I'll do some investigation on my end and see if I can implement something that works.)

@mbargiel
Copy link
Collaborator Author

mbargiel commented Nov 3, 2013

Yeeeaah, well... I spent a couple hours looking at ways to implement this.

I couldn't really find any library providing a SSLConnectionPool class without having to pull in a bunch of unnecessary dependencies (e.g. geventhttpclient). I guess we could probably copy and adapt some code from geventhttpclient or urllib3's HTTPConnectionPool (both are used under the MIT license, which is very liberal), but it would take some time to do this properly. I won't have time to do this for now.

In the meantime, I'll try to whip up some not-really-reusable-but-quick fix on my end, but I don't think I'll want to open a pull request for that..

@stephenmuss
Copy link
Owner

Thanks @mbargiel. As you pointed out, APNService.push_notification_to_devices was originally written to push notifications to large numbers of devices. However, it is evident that some flexibility is warranted.

I'd like to take a look at connection pooling/reusing. Thanks for pointing me to urllib3's HTTPConnectionPool. I'll do some investigating when I have some time.

@mbargiel
Copy link
Collaborator Author

mbargiel commented Nov 7, 2013

@stephenmuss I think it would be worth sharing my last attempt with you. I'll try to find a moment to open a pull request this weekend or the next.

@stephenmuss
Copy link
Owner

@mbargiel sounds great!

@dperetti
Copy link

At first sight, it looks like the network logic should be decoupled from the APNService model and be executed by a Celery worker. That would solve many issues, including the requirement of a persistent connection.
What do you think ?

@mbargiel
Copy link
Collaborator Author

You're right in that. As a matter of fact, we had some ideas to refactor
the service configuration (certificate, etc.) from the service interaction
(connection, etc.)

However, we probably don't want to tie this library to specific frameworks.
Celery is great but someone might want to use DIN asynchronously with
another framework.

I'm not sure what you mean about "the requirement of a persistent
connection". Could you please clarify?

Le vendredi 22 novembre 2013, dperetti a écrit :

At first sight, it looks like the network logic should be decoupled from
the APNService model and be executed by a Celery worker. That would solve
many issues, including the requirement of a persistent connection.
What do you think ?


Reply to this email directly or view it on GitHubhttps://github.com//issues/31#issuecomment-29071385
.

@dperetti
Copy link

If the network logic is refactored, I'm sure it will be easy to support Celery without requiring it.
As for the persistent connection, I was just referring to your own mention of the fact that the socket should be kept open and reused ! (which I didn't know, thanks for that !)

@mbargiel
Copy link
Collaborator Author

It definitely should be. I see the optional integration of Celery in the
DIN library as a nice to have, but it's already very easy to do on your
own, simply by wrapping calls to the notification-sending function in a
celery-decorated function. I wrote a simple Connection Pool that took care
of the connection re-use aspect. It's okay for now for my use, but I would
rather implement the network logic refactoring before integrating the pool
more robustly, perhaps even making the pool sharable between multiple
processes (like Celery workers).

Stay tuned! :)

Le vendredi 22 novembre 2013, dperetti a écrit :

If the network logic is refactored, I'm sure it will be easy to support
Celery without requiring it.
As for the persistent connection, I was just referring to your own mention
of the fact that the socket should be kept open and reused ! (which I
didn't know, thanks for that !)


Reply to this email directly or view it on GitHubhttps://github.com//issues/31#issuecomment-29091824
.

@kalvish21
Copy link

Is there any progress in this? I definitely agree that it's worth decoupling the networking from the model itself.

@mbargiel
Copy link
Collaborator Author

Not on my end, even if I brought it up in the first place. sheepish I
was planning on undertaking this task but I don't think that's realistic
anymore. Unfortunately, I no longer have time to actively contribute to the
library.

If you are interested, I could probably share the prototype code for the
connection pooling feature I was going to refactor and integrate in
Ios-notifications.

Le lundi 16 février 2015, kalvish21 notifications@github.com a écrit :

Is there any progress in this? I definitely agree that it's worth
decoupling the networking from the model itself.


Reply to this email directly or view it on GitHub
#31 (comment)
.

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

No branches or pull requests

4 participants