I've "fixed" a memory leak, but.. how to fix it in a better way?

Question

It was a very fast and makeshift, bug fix.. It worked, but I would like to find a better understanding and solution.

This was the Class Constructor generating the leak

final transient  DataInputStream din;
final transient  DataOutputStream dout;
final transient  BufferedReader bin;
final transient  BufferedWriter bout;

NData(Socket sock0) throws IOException
    {
        sock=sock0;
        din= new DataInputStream(sock.getInputStream());
        dout = new DataOutputStream(sock.getOutputStream());
        bin = new BufferedReader(new InputStreamReader(din));
        bout = new BufferedWriter(new OutputStreamWriter(dout));
     //   .. 
    }

The bug fix was to changed it (remove final) so that let me to assign null later

transient  DataInputStream din;
transient  DataOutputStream dout;
transient  BufferedReader bin;
transient  BufferedWriter bout;

NData(Socket sock0) throws IOException
    {
        sock=sock0;
        din= new DataInputStream(sock.getInputStream());
        dout = new DataOutputStream(sock.getOutputStream());
        bin = new BufferedReader(new InputStreamReader(din));
        bout = new BufferedWriter(new OutputStreamWriter(dout));
     //   .. 
    } 

 //And to add a "magic" destructor

 void nuller() {
        din=null;
        dout=null;
        bin=null;
        bout=null;
    }

There was a finish method that ended the thread, closes the streams, so I add there the "nuller" method call, and memory leak went away.

Why after finished the thread and closed the stream, it keep allocating memory in "byte[]" ? Why GC don't throw it away ? (except after that dirty with null asignment)

EDIT:

As casablanca says perhaps the NData object is still around, there is a

final static ConcurrentHashMap <String,NData>(); 

so that have NData as values, A remove(key) is done to purge the object from the Map, but.. it seems not be enough.

Removing from a HashMap the only object reference won't be enough to remove the object?

Answer 1

Why after finished the thread and closed the stream, it keep allocating memory in "byte[]" ? Why GC don't throw it away ?

The GC will "throw" something away only when there are no more references to that object. In your case, this means that something is still holding a reference to the NData object. By manually calling your nuller method, you simply release the references to the member variables (din, dout etc.) but the NData object is probably still lying around. You need to look elsewhere to find out who is using this object and make sure that this reference is cleaned up.

Update: How exactly did you come to the conclusion that there is a memory leak? The GC only runs periodically, so objects are not guaranteed to be freed immediately. You could try calling System.gc() to force a GC run. Also, ConcurrentHashMap (which I'm not familiar with) might be caching references for concurrency and it's possible that these aren't freed immediately after calling remove.

Answer 2

In your nuller call bin.close() and bout.close() That will do it. Just make sure that the nuller is called in a finally block somewhere so it always gets called.

Answer 3

You should take a heap dump after you cleaned up the NData object. Then find the NData object in the heap (because from your description, someone is still holding a strong reference to it!). Then find how is this object reachable from the root set.

Recently I had to do this, and I found the process convenient using these tools:

VisualVM, for taking the heap dump.

Eclipse Memory Analyzer (I used it standalone) to analyze the heap, in particular find the paths from the root set.

The second currently is significantly more advanced than the first, but the GUI of the first is very nice to use (if it was also feature-complete!).