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

Add synchronous calls for sending messages. #77

Open
tomekowal opened this issue Sep 23, 2015 · 5 comments
Open

Add synchronous calls for sending messages. #77

tomekowal opened this issue Sep 23, 2015 · 5 comments
Labels

Comments

@tomekowal
Copy link

Sending too many devices with wrong device ids causes closing socket by APNS server, which in turn crashes apns_connection gen_server.

The carsh is not a big deal, because apns_connection is supervised, but send function is using gen_server:cast, so when the server crashes, all messages that were queued in process mailbox are gone and we don't know, what messages were there.

My solution to this problem would be to add gen_server:call doing exactly the same thing - sending messages, but because it is synchronous, we might be sure that:

  • we know, that it crashed and we can retry sending the message after it gets restarted
  • no other messages hidden in mailbox are lost (if we are not using cast)

If you think, this is a good idea, I can make a pull request, but it would be great to fix #71 before that.
Otherwise, it is a little bit hard to test, if everything is working correctly.

If you can think of a better solution, I am open.

@elbrujohalcon
Copy link
Member

What if we somehow make the retries internal? We can queue up send requests in, say… an ets… from which the gen_server pulls messages to send.
Before/instead gen_server:cast, we insert a row in that ets table. On init, apns_connection just retrieves all messages to send from the table and starts sending them one by one.
After a message is properly sent, we just remove it from the table.
As a matter of fact I think we're actually doing something quite similar to manage errors coming back from the feedback connection.

@tomekowal
Copy link
Author

That might be a good idea. I was also thinking about simply handling the
** (exit) {:error, :closed}
Instead of stopping the server, we can handle this one type of error. This way messages in the process queue are preserved.
It aslo solves another problem we encountered. When we have 512 connections and we are sending messages quickly through all of them, it is very probable, that we exceed number of restarts and crash entire apns application.
Handling this one type of error by trying to reconnect with the same gen_server process can solve both of those problems at once.

@elbrujohalcon
Copy link
Member

Ok, please send a PR our way but be careful to properly manage the scenario where the network is down as well (it might look similar to a disconnection from apple).

@tomekowal
Copy link
Author

One more question.
Are you comfortable with adding mockapn library as a dependency to your project?
https://github.com/pdincau/mockapn
I run the tests against it now and they pass. It will be easier to simulate broken connection with it and I can add generated key, cert and password to the repository for the tests to pass. If you don't want to depend on that library, I can perform tests locally and only commit the source code.

@elbrujohalcon
Copy link
Member

Adding that library would be great for tests! I like it

@ferigis ferigis added the 1.x label Feb 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants