search

mblawrence / javapns

Automatically exported from code.google.com/p/javapns
0 stars 0 forks source link

Automate the clearing of internal references to PushedNotification objects #88

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago 
What steps will reproduce the problem?

For efficient notification delivery I start a single PushQueue with multiple 
threads during application server startup. While reading though the source I 
saw that the NotificationThread stores each delivered notification in its 
"notifications" list. Since the threads are never terminated during application 
server lifetime these instances will be never released. At some point this will 
lead to OutOfMemory errors.

Since the PushedNotifications can't be accessed through the PushQueue interface 
they shouldn't be stored.

My tests indicate that there is a similar leak in 
PushNotificationManager.sendNotification() (see attached screenshot - most of 
the LinkedHashMap$Entry instances are created in this method). It seems the 
pushedNotifications map is never cleared.

Here is the code used in my test:

        System.out.println("Starting");

        final PushNotificationPayload payload = PushNotificationPayload.complex();

        payload.addAlert("Hello World!");

        final PushQueue queue = Push.queue(keyStoreFromResource("push_dev.p12", KEYSTORE_PASSWORD), KEYSTORE_PASSWORD, false, 10);

        for(int i=0 ; i<10000 ; i++) {
            queue.add(payload, DEVICE);
        }

        System.out.println("Finished");

Attached is the heap content of this test program (only showing strong 
referenced objects).

What version of the product are you using? On what operating system?

javapns 2.1

Original issue reported on code.google.com by thomas...@googlemail.com on 29 Nov 2011 at 3:42

Attachments:

GoogleCodeExporter commented 8 years ago 
Indeed, there is a memory leak there.  I will add a "clear" method to the 
PushQueue interface and the NotificationThreads and NotificationThread classes 
so that the internal list can be cleared manually.

I'm not sure how we could automate the clearing of those objects though, 
because we cannot know when or how developers will use that list, meaning that 
we cannot determine when those objects will become obsolete and safe to clear.  
Clearing it manually seems to be the only safe way of doing things here.

I will fix this ASAP.  Thank you!

Original comment by sype...@gmail.com on 29 Nov 2011 at 7:19

GoogleCodeExporter commented 8 years ago 
Thank you for your quick reply.

I thought about your suggested solution and I came up with some potential 
problems. Consider the following code:

1 PushQueue q = ...;
2 List<PushedNotification> failed = q.getFailedNotifications();
3 processFailed(failed);
4 q.clear();

Since the queue continues running additional failed notification might be added 
to the list after line 2 is executed which will be also cleared from the list 
in line 4.

Obviously there are ways around this (e.g. passing a list of 
PushedNotifications to remove from the list to clear()). But it still forces 
the caller to regularly call clear().

What do you think of the following solution: return a 
Future<PushedNotification> from PushQueue.add(). If the caller needs the 
delivery result he can just call Future.get() and block the thread until the 
message is delivered. If the caller ignores the result the Future just goes out 
of scope and is gc'ed as soon as the message is delivered.

Original comment by thomas...@googlemail.com on 30 Nov 2011 at 1:09

GoogleCodeExporter commented 8 years ago 
This is an interesting solution.  The problem I see with it though is that it 
involves changing a lot of existing methods and behaviours, which will 
undoubtedly affect existing code using the library.

I would be tempted to keep your suggestion as an enhancement request for the 
future, and consider the new clearPushedNotifications() methods I have just 
added in revision 340 (2.1.1a1) as an immediate but temporary fix.

Original comment by sype...@gmail.com on 30 Nov 2011 at 1:58

GoogleCodeExporter commented 8 years ago 
Your solution solves my problem, so I won't complain ;)

Original comment by thomas...@googlemail.com on 30 Nov 2011 at 2:43

GoogleCodeExporter commented 8 years ago 
Outstanding!  I will modify this issue report accordingly.

Original comment by sype...@gmail.com on 30 Nov 2011 at 3:17

GoogleCodeExporter commented 8 years ago 
As per previous discussion, revision 340 includes a preliminary fix (new 
clearPushedNotifications methods) for the issue initially reported, but a more 
automated way of clearing references to PushedNotification objects is desirable.

One possible solution that should be considered for a future version would be 
to use the java.util.concurrent.Future<T> async computation result support.  
Since adopting that strategy would involve changing a lot of existing methods 
and behaviours (which will directly affect library users), this enhancement 
should probably be pushed to a later major milestone.

Keeping this enhancement request open for future references...

Original comment by sype...@gmail.com on 30 Nov 2011 at 3:26

GoogleCodeExporter commented 8 years ago 
Version 2.2 Beta 1 has been committed to the trunk, and includes changes that 
are related to this issue.  

As of r349, most "List<PushedNotification>" declarations have been changed to 
"PushedNotifications" (a new class which extends Vector).  The new 
PushedNotifications class supports a maxRetained property which controls how 
many PushedNotification objects it can hold at any given time.  If the library 
attempts to add to the list more objects than its maxRetained number, older 
objects are removed before new ones are added.  By default, PushedNotifications 
lists will retain a maximum of 1000 individual notifications, but this can be 
customized.

Original comment by sype...@gmail.com on 11 Jan 2012 at 10:18

GoogleCodeExporter commented 8 years ago 
Since the changes included in r349 ensure by default that no more than 1000 
references to PushedNotification instances will be kept (although this can be 
customized), the previous issue of an uncontrolled memory leak is believed to 
be fixed. 

Closing as fixed in 2.2 Beta 1.

Original comment by sype...@gmail.com on 14 Jan 2012 at 10:17