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

[Feature request] Connection pool #44

Closed
tamakisquare opened this issue May 26, 2017 · 3 comments
Closed

[Feature request] Connection pool #44

tamakisquare opened this issue May 26, 2017 · 3 comments

Comments

@tamakisquare
Copy link

Excerpt from the official docs on Best Practices for Managing Connections,

Keep your connections with APNs open across multiple notifications; do not repeatedly open and close connections. APNs treats rapid connection and disconnection as a denial-of-service attack. You should leave a connection open unless you know it will be idle for an extended period of time—for example, if you only send notifications to your users once a day, it is acceptable practice to use a new connection each day.

I think it's the best for apns2-client library to conform to this best practice, in order to avoid the possibility of having the services built with the library being seeing as a denial-of-service attack and thus interruption to those services. Therefore, I think we should add a connection pool for managing the reuse of open connections to this library.

Thanks for developing this library. It has been working great for me and its code is easy to read and follow.

@vinnyspb
Copy link

But does the library actually open-close connections constantly? My understanding was that connection is left opened after the first push, and then you can reuse it as many times as you want from as many threads as you want.

Thread-safety assumption comes from hyper package changelog (used for HTTP20Connection class):

The HTTP20Connection object is now thread-safe, so long as stream IDs are used on all method calls.

And as it is a HTTP/2 connection, library heavily reuses streams inside a single connection, which per my understanding is not the same as a connection pool of course, but gives you similar benefits.

Maybe the author could comment more on that.

@tamakisquare
Copy link
Author

@vinnyspb, good point! I missed out the fact that PyAPNs2 relies on http20, so I was expecting to see code managing connection at the PyAPNs2 library level and I didn't see any. As it turns out, I checked out the source code of the http20 package after seeing your comment, the connection management has already been well taken care of by http20. Thanks for your insight.

@avium
Copy link

avium commented Dec 7, 2018

My understanding was that connection is left opened after the first push, and then you can reuse it as many times as you want from as many threads as you want.

I'm concerned there may be thread-safety issues. For example, send_notification_batch() doesn't seem to take into account that there may be open streams pending from other calls to that method on other threads. If I'm not mistaken, this could result in too many (or more than __max_concurrent_streams) simultaneous requests.

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

No branches or pull requests

3 participants