Redth / PushSharp

A server-side library for sending Push Notifications to iOS (iPhone/iPad APNS), Android (C2DM and GCM - Google Cloud Message), Windows Phone, Windows 8, Amazon, Blackberry, and (soon) FirefoxOS devices!
Other
4.39k stars 1.52k forks source link

Iphone Retries #144

Closed roeaharoni closed 11 years ago

roeaharoni commented 11 years ago

we are getting complaints from our iPhone users that they get a lot of the time the same notifications twice and even more. when we investigated this in our servers we've seen in the logs that we've only sent it once (we log every event we get from push sharp, including their Token).

one of the possible explanations i can think of is that the notification was sent twice to APNS because of a retry. does it make sense? is it possible to happen? we send millions of notifications a day, and sometimes we have peak hours with many notifications being set at the same time, so it might be on high load when it retries to send notifications again even when it is not needed

and anyway - can you add an event to notify when a notification is retried to be send? it can be very helpful to monitor the service behavior

Thanks, Roei

Redth commented 11 years ago

There is no way currently, but I like the idea of adding an event to track it. I'll do that, thanks.

A notification shouldn't be 'retried' for iOS unless a network connection failed, or a notification was sent 'after' a failed notification was sent that Apple sends feedback for. So this is interesting that you're experiencing this! Perhaps adding this event in and having you do a bit of tracking will highlight the issue. I'll get this one out in a new release shortly!

roeaharoni commented 11 years ago

Thanks again for your help!

do you have any estimation about when such an event might be available? it'll help us to investigate this issue

we are checking another lead on it, with similar complaints from Gmail for iPhone users (http://productforums.google.com/forum/#!topic/gmail/x646jPef1xQ)

BaesFr commented 11 years ago

It's available in the source and even in the nuget version.

push.OnNotificationRequeue += NotificationRequeue;

private void NotificationRequeue(object sender, NotificationRequeueEventArgs e)
{
     _log.Debug("Requeue: " + sender + " -> " + e.Notification);
}
Redth commented 11 years ago

@BaesFr :+1:

roeaharoni commented 11 years ago

Thanks, that was quick i :) :+1: didn't notice the commit

so we'll use it and see if this is what causing us this behavior thanks

Redth commented 11 years ago

Please reopen if you have more info :)

bitsdisasters commented 11 years ago

Hi there,

I'm experiencing the same issue. The notifications are being sent until 3 or 5 times and we are sending just one for sure. We use this library only to send notifications to iPhone users.

Redth commented 11 years ago

@bitsdisasters need more info than that... What's the output of the events, and what code are you using to send the notifications.

bitsdisasters commented 11 years ago

Hi, That's the code I'm using to send notifications:

InitPush(); // --   instantiate PushBroker as Singleton:  _push = new PushBroker();
var payload = GeneratePayload(data); // --- using payload.CustomItems.Add()....
bool ProductionEnvironment = Moonbooster_Models.Parameter.GetParameter<bool>("ProductionEnvironment");
var appleCertData = Resources.PushNotificationCert;                
            _push.RegisterAppleService((new ApplePushChannelSettings(ProductionEnvironment, appleCertData, "passdd")));

            foreach (var device in devices)
            {
                if (!String.IsNullOrEmpty(device.Key))
                {

                    _push.QueueNotification<AppleNotification>(new AppleNotification()
                    .ForDeviceToken(device.Key)
                    .WithPayload(payload));
                }
            }

This is called once but taking a look into the library code and debugging it, I watched that while executing foreach statement in QueueNotification function is being called two or three times because registeredServices[pushNotificationType] is registering the Push Service more than once (public void RegisterService(IPushService pushService) where TPushNotification : Notification)

   public void QueueNotification<TPushNotification>(TPushNotification notification) where TPushNotification : Notification
    {
           var pushNotificationType = typeof (TPushNotification);

        if (registeredServices.ContainsKey(pushNotificationType))
           registeredServices[pushNotificationType].ForEach(pushService => pushService.QueueNotification(notification));

        else
       .....
    }

And another thing to say it's that after sometime, my notification service is stopped without any exception while I can watch that memory is growing when it sending notifications to iOS devices unlike Android devices (no PushSharp code is using for that). If notifications for iOS devices is disable (parameter) the service keeps running without notice. Is there any way to enable a logger within this library?. Thank you very much for you effort and time.

satishkumar4all commented 11 years ago

Hello Redth,

I am experiencing the same issue. The notifications are being sent multiple times and we are sending just one for sure. We use this library to send notifications to iPhone & iPad users.

bitsdisasters commented 11 years ago

Hi again,

I caught the issue about sending same notification several times and it was my fault. When I moved my code to instantiate the PushBroker as Singleton I forgot to move the code around registering the service so it was called every time I needed to send new notifications. Now the code has become to look like this:

internal static void InitPush()
    {
        if (_push == null)
        {
            InitLogger();
            _logger.Info("PushBroker from PushSharp Library Instantiated (static)");
            _push = new PushBroker();
            bool ProductionEnvironment = Models.Parameter.GetParameter<bool>("ProductionEnvironment");
            var appleCertData = Resources.PushNotification;
            _push.RegisterAppleService((new ApplePushChannelSettings(ProductionEnvironment, appleCertData, "passwd")));
        }
    } 

This inits the Push Broker and then I can send notifications without getting them duplicated

 internal static int Send(IList<CustomerDevice> devices, IDictionary<String, IDictionary<String, Object>> data)
    {
        InitLogger();
        InitPush();
        try
        {
            var payload = GeneratePayload(data);               

            foreach (var device in devices)
            {
                if (!String.IsNullOrEmpty(device.Key))
                {
                    payload.Badge = device.BadgeCount > 0 ? device.BadgeCount : 1;
                    _push.QueueNotification<AppleNotification>(new AppleNotification()
                    .ForDeviceToken(device.Key)
                    .WithPayload(payload));

                }
            }

        }
        catch (Exception ex)
        {
            _logger.Error("EXCEPTION: " + ex + Environment.NewLine + ex.InnerException);
            return Notifications.Notification.SendAPNError;
        }
        return Notifications.Notification.SendSuccess;
    }

I still can see that my worker is taking too much memory when it starts to send notifications for iOS devices until, at some point, the service just is stopped without exceptions, so I will look for more info about this issue.

Thank you very much

Redth commented 11 years ago

@satishkumar4all please make sure you aren't registering the apple service multiple times.

@bitsdisasters thanks for being thorough and tracking this one down. This new 'feature' in the push broker does allow for the possibility to inadvertently do this, and it's not obvious unless you look at how the code works. I might consider throwing an exception by default if you've registered multiple times, and maybe a setting to skip the exception if this behaviour is desired, so that the default will protect people unless they really know what they're doing and intentionally allow it.

I'm closing this issue for now, @bitsdisasters please feel free to open a new one if you find out more about memory usage or leaking. Thanks!

satishkumar4all commented 11 years ago

I am still getting notifications for multiple times. Below is the code I am using to send notification to iPhone/iPad

    PushBroker iPhonePush = new PushBroker();
    PushBroker iPadPush = new PushBroker();

    /// <summary>
    /// To start apple push notification service
    /// </summary>
    public void StartPushService(bool sandbox)
    {
        PushServiceSettings pushServSettings = new PushServiceSettings();
        pushServSettings.AutoScaleChannels = false;
        pushServSettings.Channels = 1;            

        if (System.IO.File.Exists(@"E:\Smart Apps Certificates\SmartApps.p12")) 
        {                
            //Wire up the events for all the services that the broker registers
            iPhonePush.OnNotificationSent += NotificationSent;
            iPhonePush.OnChannelException += ChannelException;
            iPhonePush.OnServiceException += ServiceException;
            iPhonePush.OnNotificationRequeue += NotificationRequeue;
            iPhonePush.OnNotificationFailed += NotificationFailed;
            iPhonePush.OnDeviceSubscriptionExpired += DeviceSubscriptionExpired;
            iPhonePush.OnDeviceSubscriptionChanged += DeviceSubscriptionChanged;
            iPhonePush.OnChannelCreated += ChannelCreated;
            iPhonePush.OnChannelDestroyed += ChannelDestroyed;

            iPhonePush.RegisterAppleService(new ApplePushChannelSettings(!sandbox, @"E:\Smart Apps Certificates\SmartApps.p12", "pwd"), pushServSettings);

        }
        if(System.IO.File.Exists(@"@"E:\Smart Apps Certificates\SmartApps_ipad.p12"))
        {
               //Wire up the events for all the services that the broker registers
            iPadPush.OnNotificationSent += NotificationSent;
            iPadPush.OnChannelException += ChannelException;
            iPadPush.OnServiceException += ServiceException;
            iPadPush.OnNotificationRequeue += NotificationRequeue;
            iPadPush.OnNotificationFailed += NotificationFailed;
            iPadPush.OnDeviceSubscriptionExpired += DeviceSubscriptionExpired;
            iPadPush.OnDeviceSubscriptionChanged += DeviceSubscriptionChanged;
            iPadPush.OnChannelCreated += ChannelCreated;
            iPadPush.OnChannelDestroyed += ChannelDestroyed;

            iPadPush.RegisterAppleService(new ApplePushChannelSettings(!sandbox, @"E:\Smart Apps Certificates\SmartApps_ipad.p12", "pwd"), pushServSettings);
        }

    /// <summary>
    /// To queue apple push notification
    /// </summary>
    /// <param name="devtoken">Device Token</param>
    /// <param name="message">Message</param>
    public bool SendNotification(string devToken, string message, string osName, bool sandbox)
    {
        try
        {

            if (osName.Contains("IPhone"))
            {
                //Fluent construction of an iOS notification        
                iPhonePush.QueueNotification(new AppleNotification()                                      
                .ForDeviceToken(devToken)
                .WithAlert(message)
                .WithSound("default")
                .WithBadge(1));
            }
            else if (osName.Contains("IPad"))
            {
                //Fluent construction of an iOS notification        
                 iPadPush.QueueNotification(new AppleNotification()     
                .ForDeviceToken(devToken)
                .WithAlert(message)
                .WithSound("default")
                .WithBadge(1));

            }
            return true;
        }
        catch
        {
            return false;
        }

    }