Critical exceptions should be thrown

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Start NotificationThreads behind a corporate firewall

What is the expected output? What do you see instead?
1. I do expect there to be a ConnectException.  The problem is that it is sent 
to console output, but there doesn't appear to be notification to the program 
that this occurred (I have a try/catch around my complete code block and 
nothing was thrown).

What version of the product are you using? On what operating system?
2.0 on Windows XP.

Please provide any additional information below.
I understand the idea behind not throwing exceptions so that single 
notification issues don't derail other possibly successful ones.  But when you 
can't even connect to the service to begin with, I would assume that needs to 
be reflected back to the calling program at some point.  No PushedNotifications 
were delivered back to my program so there was nothing for me to interrogate.  
The only indication I had that there was a problem was the output on my 
console.  Is there some other way that I can be alerted to this error?  Are 
there other exceptions like this that may occur that I would not be able to 
capture either?

Here is my test method body with tokens replaced here for brevity.  When I ran 
the test, it was with valid tokens. 

        try {

            /* Gather communication details */ 
            AppleNotificationServer server = new AppleNotificationServerBasicImpl("C:\\Mobile\\Development\\APNs\\ApplePushCertificates.p12", "abcd", false);

            /* Prepare a simple payload to push */      
            PushNotificationPayload payload = PushNotificationPayload.complex();
            payload.addAlert("You have new messages.");
            payload.addBadge(1);

            List<Device> deviceList = new ArrayList<Device>(3);
            for (int i = 0; i < 3; i++) {
                String tokenToUse;
                if(i==0) {
                    tokenToUse = "xxx";
                }
                else if(i==1) {
                    tokenToUse = "yyy";
                }
                else {
                    tokenToUse = "zzz";
                }
                deviceList.add(new BasicDevice(tokenToUse));
            }        

            /* Create 3 threads to spread the work evenly */ 
            NotificationThreads work = new NotificationThreads(server, payload, deviceList, 3);  

            /* Start all work threads simultaneously */ 
            work.start();

           /* Wait for all threads to finish their work */ 
            work.waitForAllThreads();

           /* Get a list of all pushed notifications */ 
            List<PushedNotification> notifications = work.getPushedNotifications(); 

            for(PushedNotification notification: notifications) {
                if (notification.isSuccessful()) {
                    /* Apple accepted the notification and should deliver it */  
                    System.out.println("Push notification sent successfully to: " +
                                                    notification.getDevice().getToken());
                } 
                else {
                    String invalidToken = notification.getDevice().getToken();
                    System.out.println("Push notification failed for: " + invalidToken);

                   /* If the problem was an error-response packet returned by Apple, get it */  
                    ResponsePacket theErrorResponse = notification.getResponse();
                    if (theErrorResponse != null) {
                        System.out.println("ResponsePacket errorResponse= " + theErrorResponse.getMessage());
                    }
                }               
            }

        }
        catch(Exception e) {
            System.out.println("Exception e=" + e);
            e.printStackTrace();
        }

I also have questions about when to use this method of creating notification 
threads vs. connection pooling and queuing the requests to be picked up by the 
pool.  I didn't see a discussion group to ask such a question.  Should I open 
an issue with the question or is there some other way I can get information 
about that?

Thanks!

Original issue reported on code.google.com by petend...@gmail.com on 9 Nov 2011 at 6:09

[GoogleCodeExporter]

Indeed, the exception management can be problematic the way it is designed 
right now.  I'll see what I can do and I'll let you know every shortly when it 
is fixed.  Thanks for your report!

As for the difference between the LIST and QUEUE modes (see javadoc for 
javapns.notification.transmission. NotificationThread.MODE), it depends on how 
your software generates notifications...  If your software has a known list of 
notifications to push in one batch, the LIST mode would be the preferred way to 
go because the library will open connections, push your notifications as fast 
as possible, and close the connections.  If on the other hand your software has 
random single notifications to push (such as when you need to notify a specific 
user about something), then using the QUEUE mode ensures that you won't be 
constantly opening and closing connections for single notifications (which is 
something that Apple warns against in their documentation).

Original comment by sype...@gmail.com on 9 Nov 2011 at 8:24

• Changed title: Critical exceptions should be thrown
• Changed state: Started
• Added labels: Priority-High
• Removed labels: Priority-Medium

[GoogleCodeExporter]

Ok, I have completely revised the way exceptions are managed and propagated.  
Now, communication and keystore problems will throw CommunicationException and 
KeystoreException that will propagate out of the library.  Other exceptions 
that are specific to individual push notifications will continue to be noted 
passively in PushedNotification objects so not to disrupt other notifications 
being pushed.

Now, for NotificationThreads, this is a little more tricky.  Individual threads 
can experience CommunicationException and KeystoreExceptions, but since they're 
running asynchronously from your own code, there needs to be a way to be aware 
of those critical exceptions, both at the individual thread level and at the 
thread coordination level.  Thus, the way I've designed it today is that any 
NotificationThread that experiences a critical exception will do two things:  
memorize it for future examination (get it later using 
notificationThread.getCriticalException()), and notify a 
NotificationProgressListener if any is attached.  Now, at the coordination 
level, a new getCriticalExceptions() method has been added to 
NotificationThreads to collect critical exceptions from all worker threads (if 
any occurred).  Finally, for easy usage of all this, a new variant to the 
waitForAllThreads() method has been added with a boolean parameter indicating 
whether to throw critical exceptions if any occur:  
notificationThreads.waitForAllThreads(throwCriticalExceptions);

So, to summarize, invoke notificationThreads.waitForAllThreads(true) instead of 
the original no-parameter method and an exception will be thrown if any thread 
experienced a critical error such as CommunicationException or 
KeystoreException.

All these changes are included in 2.1 Beta 2 which you can get from SVN.

Please let me know if this solves the issue you raised.

Thank you!

Sylvain

Original comment by sype...@gmail.com on 9 Nov 2011 at 11:04

[GoogleCodeExporter]

Thanks for the quick reply Sylvain.  I'll pull the latest and try it out and 
let you know.

Pete

Original comment by petend...@gmail.com on 10 Nov 2011 at 4:40

[GoogleCodeExporter]

I just committed a new build, so if you want to try the very latest version, 
you might want to download the version currently in the trunk.  

The new build includes a much simpler way of using multithreading...  just call 
Push.payload(yourPayload, keystore, password, production, 3, yourDevices) to 
start 3 coordinated threads to push your list of notifications.  The method 
automatically waits for threads to finish, so when it returns, it does contain 
a list of all PushedNotifications (unless a critical exception was thrown of 
course).

The latest build also includes a very easy way of creating a queue:   
Push.queue(keystore, password, production, 1) .. where 1 is the number of 
threads to create for the queue.  This method returns a PushQueue object, whose 
add(..) methods you can call to push notifications asynchronously over the 
permanent connection(s) of the queue.

Btw, your original code generated three malformed device tokens which the 
library should prevent you from using...

Hey, are you the developer that met with Renaud a few days ago? :)

Original comment by sype...@gmail.com on 11 Nov 2011 at 3:35

• Changed state: AwaitingFeedback

[GoogleCodeExporter]

OK, I liked the simplicity of the new call that you documented in your last 
comment.  I tried the following from behind our corporate firewall:         

List<PushedNotification> notifications = 
        Push.payload(payload,"C:\\PushCertificates.p12","abcd",false,1,deviceList)

The call appears to hang and not come back.  

However, when I tried Push.payloads(....), that did appropriately throw the 
CommunicationException.  But I prefer the call that is hanging now because it 
lets me specify a number of concurrent threads to execute.

Thoughts?

Original comment by petend...@gmail.com on 14 Nov 2011 at 11:13

[GoogleCodeExporter]

Could you provide a stack trace for the thread that is hanging, so that I can 
understand where the code is blocked?  Thank you!

Original comment by sype...@gmail.com on 14 Nov 2011 at 11:28

[GoogleCodeExporter]

This is odd.  The thread that is started marked "Thread [JavaPNS  grouped 
notification thread in LIST mode]" ends after what is probably the amount of 
time it needs to wait for the socket ConnectionException to be thrown, but 
control doesn't seem to be passed back to my program.  No exception is thrown, 
nor does control come back to the calling program at all. 

Hopefully that helps you narrow it down a bit more.

Original comment by petend...@gmail.com on 15 Nov 2011 at 3:45

[GoogleCodeExporter]

Hi.  It would be helpful to have stack traces (dump) for all active threads, as 
well as a complete copy of the log output (debug mode enabled).  Without that, 
it is difficult to understand what is going on and I can't help much.

In your test, how many devices do you have in your list and how many threads 
are you creating?

Needed:
1) stack traces (dump) for all active threads
2) copy of the log output

Original comment by sype...@gmail.com on 15 Nov 2011 at 4:10

[GoogleCodeExporter]

Wait, I think I found what the problem is.  I'll prepare a fix and get back to 
you.

Original comment by sype...@gmail.com on 15 Nov 2011 at 4:34

[GoogleCodeExporter]

Original comment by sype...@gmail.com on 15 Nov 2011 at 4:34

• Changed state: Started

[GoogleCodeExporter]

Ok, I found and fixed the problem.  The cause was that in very recent builds, 
the NotificationThread class was changed from a Thread to a Runnable, but the 
actual underlying Thread objects created were not properly linked to their 
ThreadGroup (the parent NotificationThreads object).  Consequently, when the 
worker threads finished working, they would not notify the parent 
NotificationThreads because they didn't know about it.

The fix is in revision 310 (JavaPNS 2.1 Beta 2.005) which you can download 
right now.

Original comment by sype...@gmail.com on 15 Nov 2011 at 4:49

• Changed state: Fixed

[GoogleCodeExporter]

Yes, I just downloaded the latest from SVN and now Push.payload(...) that I 
wanted to use behaves the same as NotificationThread's waitForAllThreads(true) 
in that they return control to the calling program, throwing the expected 
Exception.

Thanks for your help.

Original comment by petend...@gmail.com on 15 Nov 2011 at 5:32

[GoogleCodeExporter]

Outstanding!  Thanks!

Original comment by sype...@gmail.com on 15 Nov 2011 at 6:42

[GoogleCodeExporter]

What do you suggest us to call when it is running in QUEUE Mode ? 

I see the catch block in runQueue method

} catch (KeystoreException e) {
            this.exception = e;
            if (listener != null) listener.eventCriticalException(this, e);
        } catch (CommunicationException e) {
            this.exception = e;
            if (listener != null) listener.eventCriticalException(this, e);
        }

What happens when a CommunicationException or KeystoreException exception is 
occured, it simply calls the Listener's method. When this thread will again 
start processing the messages ? I see when this happens, it will never comeback 
to runQueue method. When you call NotificationThreads.add method you are adding 
messages to all the threads irrespective it is running or not. This could endup 
in accumulating messages on a NotificationThread which has 
communicationException and it is not running. This lead to OutOfMemory in my 
case. 

Please suggest what needs to be done here.

Original comment by dsreddy...@gmail.com on 7 Jan 2014 at 2:48