Queue results are not thead safe

[GoogleCodeExporter]

I want to use a queue since apple strongly recommends you DO NOT connect for 
each push. (This should be noted in the examples.) They will throttle your 
connections, so using the Push helper methods is strongly advised against. They 
claim they can even block further connections.

In using queues, I noticed that getPushedNotifications() is not thread safe. 
While the underlying Vector IS thread safe, getting the results and clearing 
are two operations. Any number of request could have finished in that time, 
resulting in lost PushedNotification objects.

The NotifcationTheads class is even worse:

    public List<PushedNotification> getPushedNotifications() {
        int capacity = 0;
        for (NotificationThread thread : threads)
            capacity += thread.getPushedNotifications().size();
        List<PushedNotification> all = new Vector<PushedNotification>(capacity);
        for (NotificationThread thread : threads)
            all.addAll(thread.getPushedNotifications());
        return all;
    }

Here the call to size() gives a result that won't be the same when addAll() is 
then called. (It will still work, but why even bother pre-allocating the 
capacity if it's going to be wrong?)

You are going to have to do some sort of synchronization to ensure this works 
consistently. Then getPushedNotifications should be an atomic 
getAncClearPushedNotifications that returns the result and clears in a single 
synchronized block.

I've modified a copy of the source to remove the get/clear methods and add this:

    public List<PushedNotification> copyAndClear();

I then added this to PushedNotifications:

    public synchronized List<PushedNotification> copyAndClear() {
        List<PushedNotification> l = new Vector<PushedNotification>();
        l.addAll(this);
        clear();
        return l;
    }

and to NotificationThread:

    @Override
    public List<PushedNotification> copyAndClear() {
        return notifications.copyAndClear();
    }

and then to NotificationThreads (not thread safe for concurrent calls):

    public List<PushedNotification> copyAndClear() {
        List<PushedNotification> l = new Vector<PushedNotification>();
        for (NotificationThread thread : threads)
            l.addAll(thread.copyAndClear());
        return l;
    }

Original issue reported on code.google.com by beth...@gmail.com on 13 Apr 2012 at 8:21

[GoogleCodeExporter]

Regarding your first comment, Apple simply states that you must make sure not 
to connect/disconnect very rapidly in a way that could be considered a DoS 
attack.  Anything else is fine, and hundreds of sites routinely open and close 
connections to push notifications periodically.  You do not *need* to use the 
Queue mechanism, the other push methods are fine, especially if you have 
predefined lists of notifications to send (which the library will push over the 
same connection whenever possible).

As for your bug report, I am pushing this to the next milestone.  Thank you!

Original comment by sype...@gmail.com on 13 Apr 2012 at 9:00

• Changed state: Accepted
• Added labels: Milestone-2.3, Priority-Low
• Removed labels: Priority-Medium

[GoogleCodeExporter]

They are unclear on what "very rapidly" is, I guess. I noticed that it takes 
over 5 seconds to connect, send a message and finish up. That severely limits 
throughput. A note in the example would have saved me some trouble, otherwise 
you are limited to 10-12 per minute.

Glad I could help! Thanks for modifying this for the next release.

Original comment by beth...@gmail.com on 13 Apr 2012 at 9:04

[GoogleCodeExporter]

The wiki doc (basic push notification) clearly states that you can (and should) 
pass a list of devices instead of looping and sending individual notifications 
yourself, to allow the library to optimize connection usage.  The 5-second 
delay is necessary when closing the connection to read error-response packets, 
but obviously not after each individual push.  You can send hundreds or 
thousands of notifications a minute if you provide a list to the library.

Original comment by sype...@gmail.com on 13 Apr 2012 at 9:16

[GoogleCodeExporter]

It's not a big deal. I figure more documentation is better. I have it working 
with a queue and 10 threads. I query them from a database very frequently since 
we want people to get them ASAP, and batching would slow things down. We 
generate them at a rate of one or two a second at peak times. So, whatever on 
the documentation. I'll look forward to 2.3

Original comment by beth...@gmail.com on 13 Apr 2012 at 11:22

[GoogleCodeExporter]

Fixed in r363, which nows requires a boolean parameter indicating if the 
internal list should be cleared while getting the list of pushed notifications 
(within a synchronized block).  This version also deprecates related 
thread-unsafe methods.

Original comment by sype...@gmail.com on 24 Apr 2012 at 3:21

• Changed state: Fixed