CODEWITHSUNDEEP
javaandroidmemory-leaksandroid-asynctaskinner-classes
Mister Smith
Mister Smith

Reputation: 28168

Are inner classes inside Activities unacceptable?

A well known source of memory leaks are AsyncTasks holding a reference to the activity, for instance, when declaring the task as an inner class:

    public class MyActivity extends Activity {

        ...

        private class MyTask extends AsyncTask<Void, Void, Void> {
            ...
        }
    }

Because they have an associated background thread (or thread pool), as long as the thread is running, the reference to the activity is held even when the activity is dismissed.

Now (and correct me if I'm wrong), when the task is done, the activity will be again available for collection.

I've an activity where there are:

Could I prevent memory leaks just cancelling timers and calling removeCallbacks on the handler?

Example:

    public class MyActivity extends Activity {
        Handler handler;
        Timer timer;


        @Override
        public void onCreate(Bundle savedInstanceState) {
            super.onCreate(savedInstanceState);

            handler = new Handler();
            timer = new Timer();

            handler.postDelayed(new MyRunnable(), 10000);
            timer.schedule(new MyTimerTask(), 1000);
        }

        @Override
        protected void onDestroy() {
            super.onDestroy();

            handler.removeCallbacksAndMessages(null);
            timer.cancel();
            handler = timer = null;
        }

        private class MyTimerTask extends TimerTask {
            ...
        }

        private class MyRunnable implements Runnable {
            ...
        }
    }

I've seen such an activity in the project I'm currently working at, and I'm considering refactoring it to make the inner classes static and add WeakReferences to the activity.
The problem is that the inner classes are handy because they have access to the activity's private variables and methods, and changing them to static also forces me to change the access modifier of those members in the activity class from private to package-default, thus making them available to any other class in the same package. This also breaks symmetry, because now there are methods in the activity that are package-default, and their symmetric counterpart is still private (for instance, I'd need to make the stopFoo method in the activity available to the now static classes, then what should I do with startFoo, that is not called from those classes?). And what about the variables in the activity that need to be accessed? Should I provide a getter now?
The addition of weak references to the activity also introduces a good number of null checks in the refactored static classes, so that they only do work if the activity is still around. In addition to this, The weak reference approach might be difficult to understand by unskilled programmers in the company (moreover, they might just start using it everywhere without being really needed).

In short, the refactored activity looks ugly, while the previous one was neat. So I'm wondering whether the inner-class based design would be acceptable, knowing I don't really mind if a runnable or timer task is left running for a short time when the activity is over.

Upvotes: 2

Views: 832

Answers (1)

CommonsWare
CommonsWare

Reputation: 1006809

Now (and correct me if I'm wrong), when the task is done, the activity will be again available for collection

Correct, assuming that nothing else has a static reference to it.

Could I prevent memory leaks just cancelling timers and calling removeCallbacks on the handler?

Those steps will help. Moreover, you need to do that anyway -- it's not like your Timer will magically stop running unless you stop it.

I don't really mind if a runnable or timer task is running for a short time when the activity is over

And from a memory management standpoint, your argument is sound. While "avoid non static inner class" is a good high-level design criterion, it's not an absolute.

However, IMHO, your real problem is not with memory management, but configuration changes. When the user rotates the screen, changes languages, puts their device in car dock, etc., your activity will be destroyed and recreated. Yet, presumably, the work from the tasks is still needed, but they will be talking to the wrong activity instance. Using a retained fragment to manage those tasks helps to address this.

Upvotes: 2

Related Questions