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

Separate ConnectAndWrite to support persistent connections #6

Open
ghost opened this issue Dec 19, 2013 · 15 comments
Open

Separate ConnectAndWrite to support persistent connections #6

ghost opened this issue Dec 19, 2013 · 15 comments

Comments

@ghost
Copy link

ghost commented Dec 19, 2013

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.

@ghost ghost self-assigned this Dec 19, 2013
@viblo
Copy link

viblo commented Jan 6, 2014

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.

@ghost
Copy link
Author

ghost commented Jan 12, 2014

Awesome, I'll take a look shortly. FWIW, I left client.go alone in my feature branch and simply created a clean connection.go file for now. It cleans up a lot of old stuff, exposes the timeout as a configurable option, etc.

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()

@draaglom
Copy link
Collaborator

@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.
It's better to fire off many notifications at once and re-queue the dropped ones when a failure becomes known as in @viblo's because of the stuff described here:
http://redth.info/the-problem-with-apples-push-notification-ser

(tl;dr version: if you wait for a response every time, you get pretty bad latency and therefore bad throughput)

@draaglom draaglom mentioned this issue Apr 23, 2014
3 tasks
@ghost
Copy link
Author

ghost commented Apr 24, 2014

@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).

🍻

@viblo
Copy link

viblo commented Apr 24, 2014

Some comments / thoughts of my fork:
We have used it in production for about a month now and from what we can tell it works fine. We use it on a social media site/app for messaging and notifications. For messaging its important to have low latency for a good user experience, and the update has given us a clear improvement in latency over our old solution.

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.

@draaglom
Copy link
Collaborator

@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:
Apple's documentation contradicts me on that last point

You may establish multiple connections to the same gateway or to multiple gateway instances. If you need to send a large number of push notifications, spread them out over connections to several different gateways. This improves performance compared to using a single connection: it lets you send the push notifications faster, and it lets APNs deliver them faster.

@ghost
Copy link
Author

ghost commented Apr 28, 2014

@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.

@draaglom
Copy link
Collaborator

@anachronistic I already started ;)

I don't have a lot of free time at the moment though, so progress might be a little slow.

@draaglom draaglom mentioned this issue Apr 30, 2014
10 tasks
@draaglom draaglom assigned draaglom and unassigned ghost Apr 30, 2014
@carbocation
Copy link

Following this exchange with bated breath. Thanks for working on this, guys!

@draaglom
Copy link
Collaborator

@carbocation Good timing, I've just picked this up again!

@taylortrimble
Copy link

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. :)

@taylortrimble
Copy link

slow throughput

At my own application level... not because of this! 😛

@olegfedoseev
Copy link

Is there any news on this issue?

@viblo
Copy link

viblo commented Oct 20, 2015

@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.

@olegfedoseev
Copy link

@viblo then it looks like i'll be using you fork :) Thank you!

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

No branches or pull requests

5 participants