8

So I've found, with the MAT that I keep creating multiple Threads with every surfaceCreate

I think I need these threads, though, but this method causes multiple instances of ViewThread, as a user navigates through my app, which is a memory leak.

How can I reorganize the way my threads are created and handled so that this doesn't happen, or how can I stop the leak from occurring?

@Override
public void surfaceCreated(SurfaceHolder holder) {
    loading=false;
    if (!mThread.isAlive()){
        mThread = new ViewThread(this);
        mThread.setMenuRunning(true);
        mThread.start();
    }
}

@Override
public void surfaceDestroyed(SurfaceHolder holder) {

    if (mThread.isAlive()){ 
        mThread.setMenuRunning(false);
    }
}

I opened and navigated away from the Career Activity of my game five times, and this is what shows up on the MAT

leak

EDIT: I've since found that depending on surfaceDestroyed for the destruction of my threads is unreliable. I now call the appropriate thread-destroying calls from a different method, triggered onPause.

3 Answers3

4

You should use a WeakReference to reference the Career in your Thread. This way the reference will be cleared when there are no more hard references to the Career.

You can track all references in MAT by right clicking the career and choosing Path To GC Roots, then with all references. This will show you the path(s) to the object retained in memory. Make sure you either clear those references when you are done with the activity or make use of WeakReferences to have the GC clear them automatically.

nickmartens1980
  • 1,593
  • 13
  • 23
  • I tried this, and those multiple instances of my class are still piling up. –  Feb 09 '13 at 22:55
  • In that case you can use MAT to find the paths from the gc roots to the objects. You'll probably find another reference somewhere in a listener/observer or something. This may also be an anonymous inner class. Try using static classes in this case with a weakreference to the instance of the class. This will break the link as soon as the gc runs. You can also try to unregister the listener manually at the proper moment. If this does not work can you share a screenshot of MAT showing the paths to the GC roots? – nickmartens1980 Feb 11 '13 at 12:17
1

Inside surfaceDestroyed, you should wait to make sure that the thread stopped before you return.

You can refer to this question, for more details

@Override
public void surfaceDestroyed(SurfaceHolder holder) {
    boolean retry = true;
    mThread.setRunning(false);
    while (retry) {
        try {
            mThread.join();
            retry = false;
        } catch (InterruptedException e) {
        }
    }
}
Community
  • 1
  • 1
iTech
  • 18,192
  • 4
  • 57
  • 80
  • I tried this, and it didn't seem to work. I still saw the multiple instances of threads when viewing the dump. –  Feb 09 '13 at 21:24
1

So I fixed it by commenting one line:

@Override
public void surfaceCreated(SurfaceHolder holder) {
    loading=false;
    if (!mThread.isAlive()){
        //mThread = new ViewThread(this);
        mThread.setMenuRunning(true);
        mThread.start();
    }
}

This was also combined with the WeakReference and the SurfaceDestroyed answers. I'll test it later and determine if it's just the removal of that one line, or a combination of that and the weak reference, or the other thing, then award the answer

  • That would be correct. It is important to note the flaw in the original thinking. isAlive() requires a valid object reference and therefore is not a good indicator as to whether you need a new Thread object. isAlive() will return false if you have a valid Thread that is not active... thus creating a new Thread is !isAlive() results in the behavior you describe. The only valid way to decide if a new Thread should be added is to check for a null pointer reference. It is good practice to check for null pointers anyway :) – Fuzzical Logic Feb 12 '13 at 01:56