timehop / apns

A Go package to interface with the Apple Push Notification Service
https://godoc.org/github.com/timehop/apns
MIT License
185 stars 47 forks source link

Asynchronous send results in exiting before all notifications are sent #50

Closed taylortrimble closed 1 year ago

taylortrimble commented 9 years ago

It's clear that:

In my mind, these are all closely intertwined with throughput. Any of these can be solved very easily at the expense of throughput. With high throughput in mind, these become more challenging to address. :)

This issue is a teaser. I have some ideas, but I'm going to wait until we're finished nailing down #31 before opening that bucket of worms. That way we can avoid the API moving every which way at once.

Forgive me,

@tylrtrmbl

joelchen commented 9 years ago

Are these the reasons why my following code only sends to the first device token in an array of tokens?

sender, err := apns.NewClientWithFiles(apns.ProductionGateway, "push.pem", "push.key.pem")
payload := apns.NewPayload()
payload.APS.Alert.Body = "I am a push notification!"
tokens := []string{"521ed189c770e481652f62c83bfe47ddb35aa1c18b5a6a7224f5125aa4b49f13", "abcd189c770e481652f62c83bfe47ddb35aa1c18b5a6a7224f5125aa4b41234"}

for _, token := range tokens {
    notification := apns.NewNotification()
    notification.ID = ""
    notification.DeviceToken = token
    notification.Identifier = 0
    notification.Expiration = nil
    notification.Priority = apns.PriorityImmediate
    notification.Payload = payload
    sender.Send(notification)
}

Changing "sender.Send(notification)" to "go sender.Send(notification)" does not work as well. Found another Apple Push Notification Go library apns.go while looking for examples of sending multiple push with this library, and I will be trying it next.

taylortrimble commented 9 years ago

Try removing the "" assignment for ID, and repeating the experiment with:

And let me know what you find!

I'm guessing there could be an error with the second token.

joelchen commented 9 years ago

Hi @tylrtrmbl,

I have followed your instructions, and there is no difference. I have even included the following sample code from error handling section of current project's main Github page, and it is not logging any errors as well:

go func() {
    for f := range sender.FailedNotifs {
        fmt.Println("Notif", f.Notif.ID, "failed with", f.Err.Error())
    }
}()

With Go, a new language designed to be concurrent, I do not expect individual I/O errors to be interfering with other simultaneous I/O operations.

joelchen commented 9 years ago

Refactoring my code to the following will achieve sending to all tokens in the array:

tokens := []string{"521ed189c770e481652f62c83bfe47ddb35aa1c18b5a6a7224f5125aa4b49f13", "abcd189c770e481652f62c83bfe47ddb35aa1c18b5a6a7224f5125aa4b41234"}

for _, token := range tokens {
    sender, err := apns.NewClientWithFiles(apns.ProductionGateway, "push.pem", "push.key.pem")
    payload := apns.NewPayload()
    payload.APS.Alert.Body = "I am a push notification!"
    notification := apns.NewNotification()
    notification.ID = ""
    notification.DeviceToken = token
    notification.Identifier = 0
    notification.Expiration = nil
    notification.Priority = apns.PriorityImmediate
    notification.Payload = payload
    sender.Send(notification)
}

Using "go sender.Send(notification)" will make the performance better at around 160ms per connection versus "sender.Send(notification)" at around 900ms per connection.

nathany commented 9 years ago

@joelchen I would suggest something inbetween those two versions. Create a NewPayload for each iteration of the loop, but only create a NewClient once.

joelchen commented 9 years ago

Hi @nathany,

I have tried moving NewClientWithFiles out of the loop, and it does not work as well. The best I could do is to move NewNotification and NewPayload out of the loop.

ghost commented 9 years ago

Hi @joelchen

If the code above is the entirety of your program running in main(), it may be that the program returns before sending the 2nd notification. The Client.Send() function returns before the notification is actually sent, which happens on a separate goroutine. I believe changes on the develop address this. I forked this project a little while ago to make Client.Send() perform its work synchronously.

nathany commented 9 years ago

Thanks Courtland.

joelchen commented 9 years ago

Hi @courtf,

Looking forward to have the develop branch merged into master branch.

nathany commented 9 years ago

I'm afraid it's still a ways off. Still trying to figure out the error handling when moving to synchronous writes.