-
Notifications
You must be signed in to change notification settings - Fork 126
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
Separate ConnectAndWrite to support persistent connections #6
Comments
I have created a fork with this functionality here: https://github.com/p1cn/apns I still havent tested it very much, but I think it should work. We are in the process of rewriting a internal service which sends notifications to use this new fork and until its done and Ive seen how it works in practice with many pushes Im not considering it done as I wouldnt be surprised if there was some tweaks needed. |
Awesome, I'll take a look shortly. FWIW, I left https://github.com/anachronistic/apns/blob/persistent-connection/connection.go The interface is still pretty simple. conn := apns.NewConnection("gateway.sandbox.push.apple.com:2195")
conn.SetTimeoutSeconds(5)
conn.CreateCertificate("YOUR_CERT_PEM", "YOUR_KEY_NOENC_PEM", false)
conn.Connect()
conn.Send(notification)
// ...send a lot more notifications if desired
conn.Send(notification)
conn.Disconnect() |
@anachronistic @viblo I've had a look at both your versions and while @viblo's version is a little too big for me to take in in one sitting, I think he's got the design basically right. @anachronistic, your version preserves synchronicity (for the caller) but I don't think that's desirable. (tl;dr version: if you wait for a response every time, you get pretty bad latency and therefore bad throughput) |
@draaglom very good points. I can add some background to my thought process real quick and we can hash out a next step (or steps). I was aiming to provide enough primitives that someone could grab the package and send notifications with minimal fuss; if you needed more complex strategies (more client connections to deal with your PN volume, specific failure logic, etc.) it would be simple enough to layer that on within your application (create X connections, place channels and other structures in front of your client connections to handle retry/reconnect, etc.) I'm more than happy to approach it a different way though, if the hands-off approach puts too much responsibility on the application (specifically in terms of grokking APNS quirks). 🍻 |
Some comments / thoughts of my fork: I think the library should to take care of as much as possible (within reason), so that not everyone has to reimplement the same functionality (we already use it in several different code bases). What I see is missing now is to have more than one connection to apple, but we settled on one for now to make the implementation easier to write and understand. For the amount of push we do its fine, but if we get much more users a multi connection setup might be needed. |
@anachronistic I think that's the right way of thinking! However, I believe the synchronous send has a bit of an impedance mismatch with the underlying (asynchronous) protocol - and that makes it difficult to build on top of. Our service sends a relatively small number of push notifications - I'm talking tens per day at the moment. However, they're very bursty (like @viblo, we send messages between users so a very common use case is to notify many users at the same time) and so the choice is to either make a whole bunch of connections or have poor latency. Asynchronously sending notifications with a queue+retries on a single connection should serve the majority of use cases very well - a modest network connection should be able to send many thousands of notifications per second. I'd also (but in no way from experience) speculate that as you move past the limits of a single connection you would more likely scale up multiple machines rather than multiple connections on a single machine - so multiple connections probably isn't something that needs worrying about. Edit:
|
@draaglom right on, I gotcha. Do you want to own the modifications? Wouldn't be the worst thing in the world to have another committer. |
@anachronistic I already started ;) I don't have a lot of free time at the moment though, so progress might be a little slow. |
Following this exchange with bated breath. Thanks for working on this, guys! |
@carbocation Good timing, I've just picked this up again! |
Me here saying I'm waiting too! Although I'm not any better off time-wise than the rest of you, so I'll just hold back, wait, and enjoy my current levels of slow throughput. :) |
At my own application level... not because of this! 😛 |
Is there any news on this issue? |
@olegfedoseev I dont have any specific updates about this repo, but my company is still using the fork I created (https://github.com/p1cn/apns) without any major issues, and I think for many use cases it will work good enough. At the moment we send out more than 5 million push messages per day spread out over a couple of servers, that means more than 1 million pushes per day from a single instance using our fork. |
@viblo then it looks like i'll be using you fork :) Thank you! |
Digging through the APNS documentation it looks like they intend for the connection to be left open, so I'll split this functionality up. The current approach will be available for one-offs and there will also be a way to keep the connection open.
The text was updated successfully, but these errors were encountered: