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

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

Closed
taylortrimble opened this issue May 17, 2015 · 10 comments
Closed

Comments

@taylortrimble
Copy link
Contributor

It's clear that:

  • Reliable delivery (and its limitations) is not generally well understood by users of the library
  • Error handling / reporting should be more clearly exposed
  • Some clients will benefit from a Nagle's-like or TCP_CORK-like implementation of sending APNS messages

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
Copy link

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
Copy link
Contributor Author

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

  • The same order of tokens
  • Reversed order of tokens

And let me know what you find!

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

@joelchen
Copy link

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
Copy link

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
Copy link
Contributor

nathany commented May 20, 2015

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

@nathany nathany changed the title Multiple Connection / High Throughput / Batching Ideas Sending the same payload to multiple device tokens May 20, 2015
@joelchen
Copy link

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
Copy link

ghost commented May 21, 2015

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
Copy link
Contributor

nathany commented May 22, 2015

Thanks Courtland.

@joelchen
Copy link

Hi @CourtF,

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

@nathany
Copy link
Contributor

nathany commented May 22, 2015

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

@nathany nathany changed the title Sending the same payload to multiple device tokens Asynchronous send results in exiting before all notifications are sent May 22, 2015
@taylortrimble taylortrimble closed this as not planned Won't fix, can't repro, duplicate, stale Apr 6, 2023
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