Possible fix for #14 Socket connection drops next notification if invalid notification is sent

[Kikimora]

Hi, I got the same problem as described in issue #14. Later on I discovered than "invalid" device tokens which trigger the problem came from Debug versions of my app. Basically Apple drops connection and does not send anything as soon as it sees token from sandbox environment send to production one. Once I removed sandbox tokens my pushes sent fine.

But this is only part of the picture. I dig a bit deeper and implemented simple error checking as described in apple docs. You can see this code in the pull request. I used IO.select trick from "Lead Zeppeling" to get error messages. And weird thing happened. Once I read errors I see that Apple correctly reports sandbox tokens as being invalid (error code 8). But Apple reports it only once! Next pushes with same tokens (including invalid once) work without connection drops and without any errors from Apple side. I do not know who to blame this for. Maybe IO.select did the trick, maybe reading error information from socket, maybe anything else...

Anyway this pull request has the code I created to read errors. And using it I was able to fix the problem.

My sending code looks like this

devices.each_with_index do |device, index|
            n = Grocer::Notification.new(device_token:device.token,
                                         alert: news_content.title,
                                         identifier: index)
            pusher.push(n)
            Rails.logger.debug "Pushing #{device.token} #{news_content.title}"
end
errors = pusher.read_errors
invalid_devices = errors.map {|error| devices[error[:identifier]]}
Rails.logger.warn "APNS send resulted in errors: #{errors}"
devices.delete_all({:id.in => invalid_devices})

[linjunpop]

I think def select(timeout) is much more readable.

[stevenharman]

This couples us directly to Rails, which we can't do.

[stevenharman]

We are unlikely to accept any pull requests without accompanying specs. Also, please try to adhere to the code style of this project - that is, we don't use the Seattle-style method defs:

# prefer
def method_name(arg = default)
  #...
end

# instead of
def method_name arg = default
  #...
end

[kbrock]

I see a bunch of style issues, between the method definitions and use of Rails logger.
Besides those, is there something that is holding up this pull request?

[stevenharman]

Hi @kbrock,
Aside from the stylistic issues, and the Rails.logger calls there are also zero specs for this change. There also seems to be a good bit of unused code, which makes me question if this actually works. We'd really love to fix the dropped-connection issue, but without any specs showing the effects of this change, I don't have confidence enough to merge it.

Would you care to take a crack at adding some specs or addressing some of the issues we've brought up?

[johnnaegle]

FWIW, I'm using @Aleph7's read_reply patch to handle issue #14 . I have this something like this (I've removed a bunch of guards and logging). The basic idea is to look for the error response packet and re-send everything in the batch after the notification that caused the error.

YMMV, but I think it illustrates that this could possibly be handled in client code

# push each Apn::Notification (my class) to apple using Grocer
to_send.each {|notification| notification.send_to_apns(pusher)}

# sleep seems to be necessary (non-blocking sockets?)
sleep(1) 
reply = pusher.read_reply
unless reply.nil?
  notification = Apn::Notification.find_by_id(reply.identifier)
  notification.update_attributes!(:error_status => reply.status)

  if reply.status == 8 #invalid token
    notification.device.update_attributes!(:enabled => false)
  end

  to_send[ to_send.find_index(notification) + 1 .. -1 ].each do |to_resend|
    to_resend.update_attributes!(:status => 'new') if to_resend.status == 'sent'
  end
end

[kbrock]

for @johnnaegle code

If I had a bad token in to_send, it only made it 2 or 3 notifications past until it blew up. If I went further than that, I was not able to determine that there was even an error, and would not know which notifications had been sent or not.

It may be the issue with the branch resetting connections on error