patric-r / jvmtop

Java monitoring for the command-line, profiler included
GNU General Public License v2.0
1.23k stars 253 forks source link

Memory leak in VMOverviewView #54

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
vmInfoList and vmMap in com.jvmtop.view.VMOverviewView are add in when new jvm 
process created, but never cleaned even if the jvm process is dead(detached), 
this may caused memory leak.

A possible fix:

diff --git a/jvmtop/src/com/jvmtop/view/VMOverviewView.java 
b/jvmtop/src/com/jvmtop/view/VMOverviewView.java
index c4ada39..4dfa89c 100644
--- a/jvmtop/src/com/jvmtop/view/VMOverviewView.java
+++ b/jvmtop/src/com/jvmtop/view/VMOverviewView.java
@@ -23,6 +23,7 @@ package com.jvmtop.view;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
@@ -82,6 +83,23 @@ public class VMOverviewView extends AbstractConsoleView
             "%5d %-15.15s [ERROR: Connection refused/access denied] %n",
             vmInfo.getId(), getEntryPointClass(vmInfo.getDisplayName()));
       }
+      else if (vmInfo.getState() == VMInfoState.DETACHED) {
+        System.out.printf("%5d %-15.15s [ERROR: Detached, will remove it from 
list] %n",
+           vmInfo.getId(), getEntryPointClass(vmInfo.getDisplayName()));
+
+        // remove from vmMap
+        vmMap.remove(vmInfo.getId());
+      }
+
+      // remove from vmInfoList
+      Iterator<VMInfo> it = vmInfoList.iterator();
+      while(it.hasNext()){
+        Integer vmid = it.next().getId();
+
+        if(vmMap.containsKey(vmid)){
+            vmInfoList.remove(it);
+        }
+      }

     }
   }

Original issue reported on code.google.com by hailinz...@gmail.com on 24 Jul 2014 at 11:41

GoogleCodeExporter commented 9 years ago
Thanks, I know what you mean - but I would not call it a leak. 

Jvmtop needs the information which PID/JVMs have been requested before in order 
to not need to request them within every refresh interval as this would 
increase cpu usage significantly.

Please note that terminated processes are not the only reason why jvmtop cannot 
connect to them. Security-related reasons are very common, too.
Your patch would fix the memory leak but with the cost that jvmtop would try to 
connect to all others processes which are still alive but could not be 
connected for some reason within every refresh interval.

Additionally, the memory increment is minor for most users/use case scenarios 
so I did a trade-off between memory and cpu usage.

However, I agree that the current solution isn't perfect - jvmtop should remove 
all Map entries for processes which have really been terminated - but keep all 
others.

Original comment by patric.r...@gmail.com on 18 Aug 2014 at 3:47

patric-r commented 9 years ago

As stated in my previous comment, this is not a bug but intended behavior. We are consuming a little amount of memory in order to save cpu usage, which IMHO is a good tradeoff.