From bd2d7e4115ee0e69e56f0be8dbe70b632b86fc76 Mon Sep 17 00:00:00 2001 From: denis borovnev Date: Mon, 14 Nov 2016 06:54:08 +0300 Subject: [PATCH 1/3] fixed apns batch sending error handling for case when response wasn't read for previous batch (due to not enough timeout) and for current batch we read error from previous batch. As a result we can't find failed notification in current batch --- PushSharp.Apple/ApnsConfiguration.cs | 3 +++ PushSharp.Apple/ApnsConnection.cs | 33 +++++++++++++++++++++++----- 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/PushSharp.Apple/ApnsConfiguration.cs b/PushSharp.Apple/ApnsConfiguration.cs index 464a5e62..1322b2f7 100644 --- a/PushSharp.Apple/ApnsConfiguration.cs +++ b/PushSharp.Apple/ApnsConfiguration.cs @@ -79,6 +79,7 @@ void Initialize (ApnsServerEnvironment serverEnvironment, X509Certificate2 certi MillisecondsToWaitBeforeMessageDeclaredSuccess = 3000; ConnectionTimeout = 10000; MaxConnectionAttempts = 3; + ResponseWaitTimeout = 750; FeedbackIntervalMinutes = 10; FeedbackTimeIsUTC = false; @@ -162,6 +163,8 @@ public void OverrideFeedbackServer (string host, int port) public int ConnectionTimeout { get; set; } + public int ResponseWaitTimeout { get; set; } + public int MaxConnectionAttempts { get; set; } /// diff --git a/PushSharp.Apple/ApnsConnection.cs b/PushSharp.Apple/ApnsConnection.cs index 0908b166..e90634c4 100644 --- a/PushSharp.Apple/ApnsConnection.cs +++ b/PushSharp.Apple/ApnsConnection.cs @@ -199,7 +199,7 @@ async Task Reader () // read (in the case that all the messages sent successfully, apple will send us nothing // So, let's make our read timeout after a reasonable amount of time to wait for apple to tell // us of any errors that happened. - readCancelToken.CancelAfter (750); + readCancelToken.CancelAfter (Configuration.ResponseWaitTimeout); int len = -1; @@ -252,10 +252,17 @@ async Task Reader () //Get the index of our failed notification (by identifier) var failedIndex = sent.FindIndex (n => n.Identifier == identifier); - // If we didn't find an index for the failed notification, something is wrong - // Let's just return + // Looks like failed index was from previous batch. The reason is that readCancelToken timeout wasn't enought + // to get response from apns. + // So put notifications from current batch again to queue and use new socket for next batches + // TODO: check that notifications from current batch weren't sent if (failedIndex < 0) + { + Log.Info("APNS-Client[{0}]: Cant find failing notification {1} in current batch", id, identifier); + + EnqueRemainingBatchItems(); return; + } // Get all the notifications before the failed one and mark them as sent! if (failedIndex > 0) { @@ -286,14 +293,19 @@ async Task Reader () // The remaining items in the list were sent after the failed notification // we can assume these were ignored by apple so we need to send them again // Requeue the remaining notifications + EnqueRemainingBatchItems(); + } + + private void EnqueRemainingBatchItems() + { foreach (var s in sent) - notifications.Enqueue (s.Notification); + notifications.Enqueue(s.Notification); // Clear our sent list - sent.Clear (); + sent.Clear(); // Apple will close our connection after this anyway - disconnect (); + disconnect(); } bool socketCanWrite () @@ -307,6 +319,15 @@ bool socketCanWrite () if (!client.Client.Connected) return false; + // looks like response for previous batch wasn't read (and some errors were in previous batch) + // Unfortunatelly we already notified client that notifications were sent. + // But at least we won't use this socket for new batch... + if (client.Available > 0) + { + Log.Info("APNS-Client[{0}]: Previous batch wasn't processed correctly. Try to increase ResponseWaitTimeout"); + return false; + } + var p = client.Client.Poll (1000, SelectMode.SelectWrite); Log.Info ("APNS-Client[{0}]: Can Write? {1}", id, p); From 3d52797b2a43cc91f3f13a5b5018c8c5962cdcb0 Mon Sep 17 00:00:00 2001 From: Denis Borovnev Date: Mon, 14 Nov 2016 11:05:51 +0300 Subject: [PATCH 2/3] fixed formatting removed TODO --- PushSharp.Apple/ApnsConnection.cs | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/PushSharp.Apple/ApnsConnection.cs b/PushSharp.Apple/ApnsConnection.cs index e90634c4..943342cf 100644 --- a/PushSharp.Apple/ApnsConnection.cs +++ b/PushSharp.Apple/ApnsConnection.cs @@ -255,12 +255,10 @@ async Task Reader () // Looks like failed index was from previous batch. The reason is that readCancelToken timeout wasn't enought // to get response from apns. // So put notifications from current batch again to queue and use new socket for next batches - // TODO: check that notifications from current batch weren't sent if (failedIndex < 0) { - Log.Info("APNS-Client[{0}]: Cant find failing notification {1} in current batch", id, identifier); - - EnqueRemainingBatchItems(); + Log.Info ("APNS-Client[{0}]: Cant find failed notification {1} in current batch", id, identifier); + EnqueRemainingBatchItems (); return; } @@ -299,13 +297,13 @@ async Task Reader () private void EnqueRemainingBatchItems() { foreach (var s in sent) - notifications.Enqueue(s.Notification); + notifications.Enqueue (s.Notification); // Clear our sent list - sent.Clear(); + sent.Clear (); // Apple will close our connection after this anyway - disconnect(); + disconnect (); } bool socketCanWrite () @@ -321,10 +319,10 @@ bool socketCanWrite () // looks like response for previous batch wasn't read (and some errors were in previous batch) // Unfortunatelly we already notified client that notifications were sent. - // But at least we won't use this socket for new batch... + // But at least we won't use this socket for new batch because otherwise new enqueued notifications won't be sent too... if (client.Available > 0) { - Log.Info("APNS-Client[{0}]: Previous batch wasn't processed correctly. Try to increase ResponseWaitTimeout"); + Log.Info ("APNS-Client[{0}]: Previous batch wasn't processed correctly. Try to increase ResponseWaitTimeout"); return false; } From 1967d6edc39436da4940c2cef994f8e1e354f022 Mon Sep 17 00:00:00 2001 From: denis borovnev Date: Sat, 3 Dec 2016 21:32:54 +0300 Subject: [PATCH 3/3] use MillisecondsToWaitBeforeMessageDeclaredSuccess instead of ResponseWaitTimeout see https://github.com/Redth/PushSharp/pull/774#issuecomment-264169337 --- PushSharp.Apple/ApnsConfiguration.cs | 5 +---- PushSharp.Apple/ApnsConnection.cs | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/PushSharp.Apple/ApnsConfiguration.cs b/PushSharp.Apple/ApnsConfiguration.cs index 1322b2f7..cdf3be0c 100644 --- a/PushSharp.Apple/ApnsConfiguration.cs +++ b/PushSharp.Apple/ApnsConfiguration.cs @@ -76,10 +76,9 @@ void Initialize (ApnsServerEnvironment serverEnvironment, X509Certificate2 certi Certificate = certificate; - MillisecondsToWaitBeforeMessageDeclaredSuccess = 3000; + MillisecondsToWaitBeforeMessageDeclaredSuccess = 750; ConnectionTimeout = 10000; MaxConnectionAttempts = 3; - ResponseWaitTimeout = 750; FeedbackIntervalMinutes = 10; FeedbackTimeIsUTC = false; @@ -163,8 +162,6 @@ public void OverrideFeedbackServer (string host, int port) public int ConnectionTimeout { get; set; } - public int ResponseWaitTimeout { get; set; } - public int MaxConnectionAttempts { get; set; } /// diff --git a/PushSharp.Apple/ApnsConnection.cs b/PushSharp.Apple/ApnsConnection.cs index 943342cf..b411d06f 100644 --- a/PushSharp.Apple/ApnsConnection.cs +++ b/PushSharp.Apple/ApnsConnection.cs @@ -199,7 +199,7 @@ async Task Reader () // read (in the case that all the messages sent successfully, apple will send us nothing // So, let's make our read timeout after a reasonable amount of time to wait for apple to tell // us of any errors that happened. - readCancelToken.CancelAfter (Configuration.ResponseWaitTimeout); + readCancelToken.CancelAfter (Configuration.MillisecondsToWaitBeforeMessageDeclaredSuccess); int len = -1;