-
Notifications
You must be signed in to change notification settings - Fork 679
Reliable Delivery #167
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
Reliable Delivery #167
Conversation
|
I think you're missing some commits here. Both I think Secondly, I am not necessarily keen on the idea of using an arbitrary timeout, as notifications which have already been purged from the cache will never receive an event. Instead I'm inclined to suggest that if this method of "reliable" delivery is enabled then when the sockets' cache fills up it will then send the sentinel. This can then be tuned by adjusting the cache size where necessary and will provide a greater degree of determinism to what is a pretty horrible situation (thanks Apple!). However, I am concerned about the performance implications of this - by forcing the socket to be torn down periodically it will greatly limit the throughput. I tend to see ~800ms startup time for the socket in which time several thousand notifications could have been sent. In cases where millions of notifications are being sent (as with several existing products using this library) that will introduce a significant time penalty. This is of course a trade off which would need to be made if reliable delivery is required, but this will naturally need associated documentation to explain. |
|
Ah, you’re right when I merged your latest changes I missed those references. If anything, this is an attempt to start the conversation about mediating some sort of “reliability guarantee.” Now that it’s on your mind, I hope there’s a good resolution on the horizon. |
|
Reliability guarantees are something I have been thinking a lot about recently with the new ECONNRESET problems people are experiencing, this would be another potential solution to mitigate them. Writing that previous message has allowed me to crystallise my ideas on this topic and I will definitely pursue it further. I'll keep this issue up to date as I develop it. I am currently ramping up to a release with the current set of changes and will look at this to include in the next version. |
|
I’m going to remove this PR to reset to the original commit against an old version of APN that seems to be working more deterministically. thanks for the discussion, and I look forward to you addressing the problem. |
|
Which version are you reverting to? |
|
still 1.4.4, but what’s in #master doesn’t seem to be sending notifications in my testing. On April 18, 2014 at 1:20:38 PM, Andrew Naylor ([email protected]) wrote: I think you're missing some commits here. Both sock and self are undefined. I have come across this idea of sending "sentinel" messages before and I'm not too keen on it. I'm coming round to the idea and if we can fettle this design a bit further I will include it. I think acknowledged notifications should only be sent when the user has specified an ackTimeout value. This way it is "guaranteed" that an acknowledged event will be fired regardless of scenario. Without the timeout set an acknowledged event will sometimes be sent, if an invalid notification is sent. Secondly, I am not necessarily keen on the idea of using an arbitrary timeout, as notifications which have already been purged from the cache will never receive an event. Instead I'm inclined to suggest that if this method of "reliable" delivery is enabled then when the sockets' cache fills up it will then send the sentinel. This can then be tuned by adjusting the cache size where necessary and will provide a greater degree of determinism to what is a pretty horrible situation (thanks Apple!). However, I am concerned about the performance implications of this - by forcing the socket to be torn down periodically it will greatly limit the throughput. I tend to see ~800ms startup time for the socket in which time several thousand notifications could have been sent. In cases where millions of notifications are being sent (as with several existing products using this library) that will introduce a significant time penalty. This is of course a trade off which would need to be made if reliable delivery is required, but this will naturally need associated documentation to explain. — |
|
Be aware of the new #master will be a 1.5.0 release as it breaks backwards compatibility, but I think it is worth doing to simplify the API and provide automatic switching using NODE_ENV if the developer desires. On 18 Apr 2014, at 21:56, Gabriel Lipson [email protected] wrote:
-A |
this commit encapsulates a few changes:
First, when a message is sent against a socket and produces an error, we know implicitly that all messages sent before it on the same socket were submitted successfully. In this case, we emit an "acknowledged" event for those messages.
Second, we provide an optional "ackTimeout" option to the constructor. If this is provided, after the timeout period (in ms), we submit a known bad token if there are any buffered messages. If this token produces an error before the previously queued messages, those messages will fire "acknowleged". This provides the consumer the ability to force acknowledgement after a specific amount of time. This timeout only fires if no other messages have already failed and interrupted the socket.