302

I am getting a warning in my code that states:

This AsyncTask class should be static or leaks might occur (anonymous android.os.AsyncTask)

The complete warning is:

This AsyncTask class should be static or leaks might occur (anonymous android.os.AsyncTask) A static field will leak contexts. Non-static inner classes have an implicit reference to their outer class. If that outer class is for example a Fragment or Activity, then this reference means that the long-running handler/loader/task will hold a reference to the activity which prevents it from getting garbage collected. Similarly, direct field references to activities and fragments from these longer running instances can cause leaks. ViewModel classes should never point to Views or non-application Contexts.

This is my code:

 new AsyncTask<Void,Void,Void>(){

        @Override
        protected Void doInBackground(Void... params) {
            runOnUiThread(new Runnable() {

                @Override
                public void run() {
                    mAdapter.notifyDataSetChanged();
                }
            });

            return null;
        }
    }.execute();

How do I correct this?

Mike M.
  • 38,532
  • 8
  • 99
  • 95
Keyur Nimavat
  • 3,595
  • 3
  • 13
  • 19
  • 3
    reading this http://www.androiddesignpatterns.com/2013/01/inner-class-handler-memory-leak.html should give you a hint of why it should be static – Raghunandan Jun 01 '17 at 13:45
  • 1
    So far, I have always been able to replace AsyncTask with new Thread(...).statr() in combination with runOnUiThread(...) if necessary, so I do not have to deal with this warning anymore. – Hong May 17 '18 at 14:16
  • 1
    What is the solution in kotlin for this problem? – TapanHP Jul 24 '18 at 06:28
  • Please reconsider which answer should be the accepted one. See answers below. – Ωmega Sep 05 '18 at 02:03
  • In my case, I get this warning from a Singleton which has no direct references to Activity (it receives the output of `myActivity.getApplication()` into the private constructor for the Singleton, in order to initialize RoomDB classes and other classes). My ViewModels get the Singleton instance as a private reference to perform some operations on the DB. So, the ViewModels import the Singleton package, as well as `android.app.Application`, one of them even `android.app.Activity`. Since "the Singleton" doesn't need to import those ViewModels to work, even so, might memory leaks occurr? – SebasSBM Nov 13 '18 at 10:42

3 Answers3

612

How to use a static inner AsyncTask class

To prevent leaks, you can make the inner class static. The problem with that, though, is that you no longer have access to the Activity's UI views or member variables. You can pass in a reference to the Context but then you run the same risk of a memory leak. (Android can't garbage collect the Activity after it closes if the AsyncTask class has a strong reference to it.) The solution is to make a weak reference to the Activity (or whatever Context you need).

public class MyActivity extends AppCompatActivity {

    int mSomeMemberVariable = 123;

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_main);

        // start the AsyncTask, passing the Activity context
        // in to a custom constructor 
        new MyTask(this).execute();
    }

    private static class MyTask extends AsyncTask<Void, Void, String> {

        private WeakReference<MyActivity> activityReference;

        // only retain a weak reference to the activity 
        MyTask(MyActivity context) {
            activityReference = new WeakReference<>(context);
        }

        @Override
        protected String doInBackground(Void... params) {

            // do some long running task...

            return "task finished";
        }

        @Override
        protected void onPostExecute(String result) {

            // get a reference to the activity if it is still there
            MyActivity activity = activityReference.get();
            if (activity == null || activity.isFinishing()) return;

            // modify the activity's UI
            TextView textView = activity.findViewById(R.id.textview);
            textView.setText(result);

            // access Activity member variables
            activity.mSomeMemberVariable = 321;
        }
    }
}

Notes

  • As far as I know, this type of memory leak danger has always been true, but I only started seeing the warning in Android Studio 3.0. A lot of the main AsyncTask tutorials out there still don't deal with it (see here, here, here, and here).
  • You would also follow a similar procedure if your AsyncTask were a top-level class. A static inner class is basically the same as a top-level class in Java.
  • If you don't need the Activity itself but still want the Context (for example, to display a Toast), you can pass in a reference to the app context. In this case the AsyncTask constructor would look like this:

    private WeakReference<Application> appReference;
    
    MyTask(Application context) {
        appReference = new WeakReference<>(context);
    }
    
  • There are some arguments out there for ignoring this warning and just using the non-static class. After all, the AsyncTask is intended to be very short lived (a couple seconds at the longest), and it will release its reference to the Activity when it finishes anyway. See this and this.
  • Excellent article: How to Leak a Context: Handlers & Inner Classes

Kotlin

In Kotlin just don't include the inner keyword for the inner class. This makes it static by default.

class MyActivity : AppCompatActivity() {

    internal var mSomeMemberVariable = 123

    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        setContentView(R.layout.activity_main)

        // start the AsyncTask, passing the Activity context
        // in to a custom constructor
        MyTask(this).execute()
    }

    private class MyTask
    internal constructor(context: MyActivity) : AsyncTask<Void, Void, String>() {

        private val activityReference: WeakReference<MyActivity> = WeakReference(context)

        override fun doInBackground(vararg params: Void): String {

            // do some long running task...

            return "task finished"
        }

        override fun onPostExecute(result: String) {

            // get a reference to the activity if it is still there
            val activity = activityReference.get()
            if (activity == null || activity.isFinishing) return

            // modify the activity's UI
            val textView = activity.findViewById(R.id.textview)
            textView.setText(result)

            // access Activity member variables
            activity.mSomeMemberVariable = 321
        }
    }
}
Suragch
  • 484,302
  • 314
  • 1,365
  • 1,393
  • @Suragch So there's possibly no way to update UI onPostExecute, if class is defined as static. – Manoj Perumarath Dec 04 '17 at 08:50
  • 1
    @ManojFrekzz, No, actually you *can* update the UI by making use of the weak reference to the Activity that was passed in. Check out my `onPostExecute` method again in the code above. You can see that I updated the UI `TextView` there. Just use `activity.findViewById` to get a reference to whatever UI element you need to update. – Suragch Dec 04 '17 at 09:56
  • 7
    +1. This is the best and cleanest solution I have ever seen! Only, in case you want to modify UI in the onPostExecute method, you should also check whether the Activity is being destroyed : activity.isFinishing() – zapotec Jan 26 '18 at 09:39
  • 1
    Note! When using this answer, I kept running into Null Pointer Exceptions because the doInBackground operation was memory intensive, triggered garbage collection, collected the weakReference, and killed the asynctask. You may want to use a SoftReference instead of a weak one if you know your background operations are more memory intensive. – PGMacDesign Feb 16 '18 at 22:10
  • In my case, I just need the fragment class because I'll be updating instance variables inside the fragment. However, when I try to create the asyntask like this: `new Task(this).execute();`, I get an error message saying that my fragment is incompatible because it's not a WeakReference version of the fragment but the real fragment instead. Have you run into this? – kriztho Mar 07 '18 at 17:07
  • @kriztho, it would be better if you asked this as a new question that showed your code. – Suragch Mar 08 '18 at 01:34
  • Android posts here on SO would really benefit from the same sort of structure and lucidity as can be seen in your answer. Both explanation and example on how to do things are thje best way to actually understand the topic at hand. Great answer! – Alexander Rossa Mar 14 '18 at 20:20
  • @Suragch nice answer man, can you tell me what should i do if I have used async class in fragment?? – Sunny Jul 09 '18 at 09:26
  • 2
    @Sunny, pass in a reference to the Fragment instead of the Activity. You would take out the `activity.isFinishing()` check and possibly replace it with a `fragment.isRemoving()` check. I haven't worked much with fragments recently, though. – Suragch Jul 10 '18 at 00:01
  • What if I want to access outer class that is not Activity and I need it in the "doInBackground"? Is calling "if (activity == null || activity.isFinishing()) return" at the beginning of "onPostExecute" is enough? what will happen if "isFinishing" will happen in the middle of "onPostExecute"? – bashan Sep 25 '18 at 16:46
  • 1
    @bashan, (1) If the outer class isn't an Activity, then in your `AsyncTask` constructor you pass in a reference to your outer class. And in `doInBackground()` you can get a reference to the outer class with `MyOuterClass ref = classReference.get()`. Check for `null`. (2) In `onPostExecute()` you are only updating the UI with the results from the background task. It is just like any other time you that you update the UI. The check for `activity.isFinishing()` is just to make sure that the activity has not already started finishing, in which case it would be pointless to update the UI. – Suragch Sep 26 '18 at 00:44
  • 1
    If you outer class is not an Activity, then in `onPostExecute()` you would only check for `null`. You wouldn't need the `activity.isFinishing()` check. – Suragch Sep 26 '18 at 00:45
  • Is there a need for the `MyActivity` to hold a reference to `MyTask`? Who retains MyTask while it is executing to make sure it isn't released before it's done? – AtomicBoolean Nov 28 '18 at 14:21
  • 1
    @ChrisWoolfe, as I understand it, since `MyTask` is an `AsyncTask` it is run on a new thread (not the UI thread). For garbage collection purposes, it is the root and is not available to be garbage collected until it finishes running. See [this answer](https://stackoverflow.com/a/2423293/3681880). Also AsyncTasks should be relatively short. So I don't think there is any danger this way. The greater danger is the other way, that the Activity will end while the `AsyncTask` still has a reference to it, hence the need for a weak reference. – Suragch Nov 28 '18 at 15:43
  • @Suragch When I change my UpdateTask extends AsyncTask<> to static, then Android Studio gives an error message on "getApplicationContext() saying its a non-static method that cannot be referenced from a static context for this line: "DatabaseClient.getInstance(getApplicationContext()).getAppDatabase() .taskDao() .update(task);" Is there a way to get the context? – AJW Feb 16 '19 at 20:01
  • 1
    @AJW, To get the context use `activityReference.get()` as in the example above. – Suragch Feb 16 '19 at 23:38
  • mSomeMemberVariable has to be static, no? – getsadzeg Apr 30 '19 at 20:16
  • 1
    @getsadzeg, No, because you use a reference to the activity instance to access it. – Suragch Apr 30 '19 at 22:07
  • 1
    Looking at the amount of buzz this topic generates in total I think they should redesign their whole activity/lifecycle/async stuff, maybe the new navigation framework already fixes this issues? – David Jul 26 '19 at 10:19
  • @Suragch is it possible to declare AsyncTask as separated class file? – Paweł Feb 17 '20 at 11:21
71

Non-static inner classes holds a reference to the containing class. When you declare AsyncTask as an inner class, it might live longer than the containing Activity class. This is because of the implicit reference to the containing class. This will prevent the activity from being garbage collected, hence the memory leak.

To solve your problem, either use static nested class instead of anonymous, local, and inner class or use top-level class.

Anand
  • 1,335
  • 1
  • 12
  • 20
  • 2
    Solution lies in the warning itself. Either use static nested class or a top level class. – Anand Jun 01 '17 at 13:59
  • 3
    @KeyurNimavat I think you can pass in a weak reference to your activity – Peter Chaula Aug 13 '17 at 12:29
  • 43
    so what is the point of using AsyncTask? if it's easier to run new Thread and handler.post or view.post (to update UI) at end in run method of Thread. If AsyncTask is static or top-level class then it's difficult to access needed variables/methods from it – user924 Oct 21 '17 at 23:58
  • 8
    there are no code provided of how is the right way to use it. I ever tried putting static there, but there will be more warning and error show up – Kasnady Feb 22 '18 at 06:00
  • 19
    @Anand Please delete this answer so the more useful answer at https://stackoverflow.com/a/46166223/145119 can be at the top. – Mithaldu Mar 25 '18 at 10:42
  • 1
    @Mithaldu, As I understand, it is not possible to delete an answer that has been accepted. Either the OP would have to deselect it as accepted or Stack Overflow would have to fix [the problem with accepted answers being pinned to the top](https://meta.stackoverflow.com/questions/326095/please-unpin-the-accepted-answer-from-the-top) This answer itself shouldn't be deleted or downvoted just because it is stuck here. I found this answer helpful and upvoted it myself even though I still needed to do the further research to write the answer that you linked to below. – Suragch Jun 19 '19 at 21:16
26

This AsyncTask class should be static or leaks might occur because

  • When Activity is destroyed, AsyncTask (both static or non-static) still running
  • If inner class is non-static (AsyncTask) class, it will have reference to the outer class (Activity).
  • If a object has no references point to it, Garbage Collected will release it. If a object is unused and Garbage Collected can not release it => leak memory

=> If AsyncTask is non-static, Activity won't release event it is destroyed => leak

Solution for update UI after make AsyncTask as static class without leak

1) Use WeakReference like @Suragch answer
2) Send and remove Activity reference to (from) AsyncTask

public class NoLeakAsyncTaskActivity extends AppCompatActivity {
    private ExampleAsyncTask asyncTask;

    @Override 
    protected void onCreate(Bundle savedInstanceState) {
        ...

        // START AsyncTask
        asyncTask = new ExampleAsyncTask();
        asyncTask.setListener(new ExampleAsyncTask.ExampleAsyncTaskListener() {
            @Override
            public void onExampleAsyncTaskFinished(Integer value) {
                // update UI in Activity here
            }
        });
        asyncTask.execute();
    }

    @Override
    protected void onDestroy() {
        asyncTask.setListener(null); // PREVENT LEAK AFTER ACTIVITY DESTROYED
        super.onDestroy();
    }

    static class ExampleAsyncTask extends AsyncTask<Void, Void, Integer> {
        private ExampleAsyncTaskListener listener;

        @Override
        protected Integer doInBackground(Void... voids) {
            ...
            return null;
        }

        @Override
        protected void onPostExecute(Integer value) {
            super.onPostExecute(value);
            if (listener != null) {
                listener.onExampleAsyncTaskFinished(value);
            }
        }

        public void setListener(ExampleAsyncTaskListener listener) {
            this.listener = listener;
        }

        public interface ExampleAsyncTaskListener {
            void onExampleAsyncTaskFinished(Integer value);
        }
    }
}
Linh
  • 57,942
  • 23
  • 262
  • 279
  • 2
    [`onDestroy()` is not guaranteed to be called every time](https://developer.android.com/reference/android/app/Activity#onDestroy%28%29) – Suragch Jun 27 '18 at 02:42
  • 5
    @Suragch your link states that, while onDestroy is not guaranteed to be called, the only situation where it isn't is when the system kills the process, so all resources are freed anyway. So, don't do saves here, but you can do a resource release here. – Angelo Fuchs Oct 31 '18 at 13:17
  • 2
    In case of non-static AsyncTask use case, why cant we just set the AsyncTask instance variable to NULL, similar to this. wont this tell GC to free Activity though AsyncTask is running? – Hanif Apr 14 '19 at 19:47
  • @Hanif setting AsyncTask instance variable to NUL will not help, because the task still has the ref through the listener. – Susanta Jun 15 '20 at 10:13