benratti / javapns

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

Possible memory leak in PushNotificationManager.pushedNotifications #120

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
What steps will reproduce the problem?
1. Using the default enhanced mode.
2. Send a lot of pushes with no errors.
3. Since there are no errors, when processedFailedNotifications is called, 
ResponsePacketReader.processResponses never has anything to do and thus 
pushedNotifications.clear() is never called.

What is the expected output? What do you see instead?
I expect it to not use 128 MB of RAM.

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

Please provide any additional information below.
Since you want to link the replies to the request, you'll need some way to 
timeout entries in pushedNotifications if no errors were reported after a few 
minutes.

It's possible I'm missing something, but I can't see how pushedNotifications 
gets cleared.

Original issue reported on code.google.com by beth...@gmail.com on 2 May 2012 at 4:37

GoogleCodeExporter commented 9 years ago
I added a breakpoint on the call to pushedNotifications.clear() in 
PushNotificationManager. It never gets hit. 

This is running in thread mode.

Maybe PushNotificationManager.initializeConnection should do this?

this.pushedNotifications = new LinkedHashMap<Integer, PushedNotification>();

Original comment by beth...@gmail.com on 2 May 2012 at 6:39

GoogleCodeExporter commented 9 years ago
Please refer to discussion in issue #88 to see if the explanations given there 
can help understand what you are seeing...

Original comment by sype...@gmail.com on 2 May 2012 at 9:17

GoogleCodeExporter commented 9 years ago
I did see that. I assumed all changes from 2.2b1 would be in 2.2 final. If so, 
the issue isn't fixed for me. The LinkedHashMap<Integer, PushedNotification> is 
never cleared, because ResponsePacketReader.processResponses(this) never 
returns a non-zero result. That is the only place this map is cleared, it it's 
never hit. Running 2.2 I've had several out of memory errors. Is it fixed in 
the 2.3 trunk, then? I can try that, I guess.

Original comment by beth...@gmail.com on 3 May 2012 at 1:00

GoogleCodeExporter commented 9 years ago
This is indeed a bug causing a memory leak.

The PushNotificationManager::sendNotification has the following line:
if (!pushedNotifications.containsKey(notification.getIdentifier())) 
pushedNotifications.put(notification.getIdentifier(), notification);

The map is only cleared in processedFailedNotifications() which is called on 
stopConnection().

So if you leave the connection open and send multiple sendNotification 
requests, there will be a memory leak. This is not fixed in the 2.3 trunk yet 
(from what I could tell).

Original comment by amir...@gmail.com on 25 Jun 2012 at 10:54

GoogleCodeExporter commented 9 years ago
I tried to send notifications to over 50,000 users, 
and I've got an exception with follow messages.

Servlet.service() for servlet jsp threw exception
java.lang.OutOfMemoryError: Java heap space
at javapns.devices.Devices.asDevices(Devices.java:22)
at javapns.Push.payload(Push.java:208)
...

I guess it is because of the same problem.

Original comment by sla...@hanmail.net on 2 Jul 2012 at 6:03

GoogleCodeExporter commented 9 years ago
Regarding the memory leak, improvements in that area should be included in an 
upcoming revision.

As for "slarin"'s issue, how much memory do you have allocated to your Java 
program?  Pushing to 50,000 tokens would obviously require a bit of memory...

Original comment by sype...@gmail.com on 2 Jul 2012 at 9:51

GoogleCodeExporter commented 9 years ago

Original comment by sype...@gmail.com on 2 Jul 2012 at 10:32

GoogleCodeExporter commented 9 years ago
I think I am having the same issue. My heapdump shows about 32768 entries in 
the map pushedNotifications. 

Original comment by ki...@crispygam.es on 25 Oct 2012 at 10:28

GoogleCodeExporter commented 9 years ago
I have the same issue. I have attached an image from JProfiler where you can 
see 2,420,179 instances of PushedNotification class. ( all of the stored in 
pushedNotifications variable from PushNotificationManager class ). The adopted 
workaround has been add the following line:

pushedNotifications.clear();  

in the stopConnection method:

public void stopConnection() throws CommunicationException, KeystoreException {
        processedFailedNotifications();
        pushedNotifications.clear();
        try {
            logger.debug("Closing connection");
            this.socket.close();
        } catch (Exception e) {
            /* Do not complain if connection is already closed... */
        }
    }

Original comment by ignavar...@gmail.com on 9 Nov 2012 at 2:03

Attachments:

GoogleCodeExporter commented 9 years ago
if i reset pushQueue very 5 minutes, will there be connection or memory leak 
issue? Like this:
every 5 minutes:
pushQueue = Push.queue(certificationPath, certificationPassword, false, 2);

Original comment by b...@benxiong.org on 13 Nov 2012 at 12:34

GoogleCodeExporter commented 9 years ago
I too have run into memory issues. I've had a degree of success solving them 
without updating the javapns. I'm running 2.2 in QUEUE mode.

I've taken to logging and clearing the pushD queue after each time i queue a 
message. 

I've wrapped the JnpsPushQueueBuilder to add JMX hooks for everything.

I've extended the PushNotificationManagerPushedNotifications to expose its 
package-protected internal queue accessor.

With respect to the connection cleanup issues, I'm going to test the latest and 
make edits to put back here. Has anyone had luck with this yet?

thanks!

matt

Original comment by m...@gogii.net on 19 Dec 2012 at 10:44

GoogleCodeExporter commented 9 years ago
I have solved the problem.
In the following methods:

public static PushedNotifications payload(Payload payload, Object keystore, 
String password, boolean production, int numberOfThreads, Object devices)

and

public static PushedNotifications payloads(Object keystore, String password, 
boolean production, int numberOfThreads, Object payloadDevicePairs)

change:
return threads.getPushedNotifications(true);

to:
PushedNotifications p = threads.getPushedNotifications(true);
threads.destroy();
return p;

The memory leak was caused because of the use of the ThreadGroup.

Original comment by giova...@brainweb.com.br on 16 Jan 2013 at 7:48

GoogleCodeExporter commented 9 years ago
Will this memory leak be fixed in coming Releases ? anyone resolved this issue ?

Original comment by dsreddy...@gmail.com on 14 Nov 2013 at 2:02

GoogleCodeExporter commented 9 years ago
@giova your solution (#12) worked great for us!  Nice work!

Original comment by jpsw...@gmail.com on 19 Jun 2014 at 5:01

GoogleCodeExporter commented 9 years ago
Fixed in r390

Original comment by sype...@gmail.com on 30 Sep 2014 at 3:43