Is AsyncTask really conceptually flawed or am I just missing something?

Question

I have investigated this problem for months now, came up with different solutions to it, which I am not happy with since they are all massive hacks. I still cannot believe that a class that flawed in design made it into the framework and no-one is talking about it, so I guess I just must be missing something.

The problem is with AsyncTask. According to the documentation it

"allows to perform background operations and publish results on the UI thread without having to manipulate threads and/or handlers."

The example then continues to show how some exemplary showDialog() method is called in onPostExecute(). This, however, seems entirely contrived to me, because showing a dialog always needs a reference to a valid Context, and an AsyncTask must never hold a strong reference to a context object.

The reason is obvious: what if the activity gets destroyed which triggered the task? This can happen all the time, e.g. because you flipped the screen. If the task would hold a reference to the context that created it, you're not only holding on to a useless context object (the window will have been destroyed and any UI interaction will fail with an exception!), you even risk creating a memory leak.

Unless my logic is flawed here, this translates to: onPostExecute() is entirely useless, because what good is it for this method to run on the UI thread if you don't have access to any context? You can't do anything meaningful here.

One workaround would be to not pass context instances to an AsyncTask, but a Handler instance. That works: since a Handler loosely binds the context and the task, you can exchange messages between them without risking a leak (right?). But that would mean that the premise of AsyncTask, namely that you don't need to bother with handlers, is wrong. It also seems like abusing Handler, since you are sending and receiving messages on the same thread (you create it on the UI thread and send through it in onPostExecute() which is also executed on the UI thread).

To top it all off, even with that workaround, you still have the problem that when the context gets destroyed, you have no record of the tasks it fired. That means that you have to re-start any tasks when re-creating the context, e.g. after a screen orientation change. This is slow and wasteful.

My solution to this (as implemented in the Droid-Fu library) is to maintain a mapping of WeakReferences from component names to their current instances on the unique application object. Whenever an AsyncTask is started, it records the calling context in that map, and on every callback, it will fetch the current context instance from that mapping. This ensures that you will never reference a stale context instance and you always have access to a valid context in the callbacks so you can do meaningful UI work there. It also doesn't leak, because the references are weak and are cleared when no instance of a given component exists anymore.

Still, it is a complex workaround and requires to sub-class some of the Droid-Fu library classes, making this a pretty intrusive approach.

Now I simply want to know: Am I just massively missing something or is AsyncTask really entirely flawed? How are your experiences working with it? How did you solve these problem?

Thanks for your input.

Enphytotic

If you're curious, we've recently added a class to the ignition core library called IgnitedAsyncTask, which adds support for type-safe context access in all callbacks using the connect/disconnect pattern outlined by Dianne below. It also allows to throw exceptions and handle them in a separate callback. See github.com/kaeppler/ignition-core/blob/master/src/com/github/… – Enphytotic

have a look at this: gist.github.com/1393552 – Enphytotic

This question is related as well. – Lebbie

I add the async tasks to an arraylist and make sure to close them all at a certain point. – Fari

Answer 1

How about something like this:

class MyActivity extends Activity {
    Worker mWorker;

    static class Worker extends AsyncTask<URL, Integer, Long> {
        MyActivity mActivity;

        Worker(MyActivity activity) {
            mActivity = activity;
        }

        @Override
        protected Long doInBackground(URL... urls) {
            int count = urls.length;
            long totalSize = 0;
            for (int i = 0; i < count; i++) {
                totalSize += Downloader.downloadFile(urls[i]);
                publishProgress((int) ((i / (float) count) * 100));
            }
            return totalSize;
        }

        @Override
        protected void onProgressUpdate(Integer... progress) {
            if (mActivity != null) {
                mActivity.setProgressPercent(progress[0]);
            }
        }

        @Override
        protected void onPostExecute(Long result) {
            if (mActivity != null) {
                mActivity.showDialog("Downloaded " + result + " bytes");
            }
        }
    }

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

        mWorker = (Worker)getLastNonConfigurationInstance();
        if (mWorker != null) {
            mWorker.mActivity = this;
        }

        ...
    }

    @Override
    public Object onRetainNonConfigurationInstance() {
        return mWorker;
    }

    @Override
    protected void onDestroy() {
        super.onDestroy();
        if (mWorker != null) {
            mWorker.mActivity = null;
        }
    }

    void startWork() {
        mWorker = new Worker(this);
        mWorker.execute(...);
    }
}

Cori

No -- mActivity will always be != null, since you're holding a strong reference to it so it won't be eligible for garbage collection. But even though it's non-null it doesn't have to be valid: if the window has been destroyed which was bound to this context, then this object is worthless, since you're trying to do UI ops on a window that doesn't exist anymore. – Enphytotic

Also, this is exactly how you create a memory leak if your task doesn't terminate. – Enphytotic

Yes, mActivity will be != null, but if there's no references to your Worker instance, then any references that instance will be subject to garbage removal as well. If your task does run forever, then you have a memory leak anyway (your task) - not to mention that you're draining the phone battery. Besides, as mentioned elsewhere, you can set mActivity to null in onDestroy. – Challah

The onDestroy() method sets mActivity to null. It doesn't matter who holds a reference to the activity before that, because it is still running. And the activity's window will always be valid until onDestroy() is called. By setting to null there, the async task will know that the activity is no longer valid. (And when a config changes, the previous activity's onDestroy() is called and the next one's onCreate() run without any messages on the main loop processed between them so the AsyncTask will never see an inconsistent state.) – Cori

true, but it still doesn't solve the last problem I mentioned: imagine the task downloads something off the internet. Using this approach, if you flip the screen 3 times while the task is running, it will be restarted with every screen rotation, and every task except the last throws its result away because its activity reference is null. – Enphytotic

one more question: the API docs for onReatainNonConfigurationInstance() read "This function is called purely as an optimization, and you must not rely on it being called." But your code relies on it being called. Is that a problem? In which cases will that method not be called? – Enphytotic

No it won't be restarted. The current AsyncTask is propagated to the new Activity instance via onRetainNonConfigurationInstance(). – Cori

Your code isn't relying on it being called -- that is, if the app is entirely restarted (process went away), you are launched without the async task. You are using it as an optimization to get to keep the same async task running across activity instances. – Cori

I see, thanks. What if I need to access the context in doInBackground()? Do I then need to declare the activity reference as volatile, since doInBackground() will be called in the worker thread? – Enphytotic

To access in the background, you would either need to put appropriate synchronization around mActivity and deal with running into times when it is null, or have the background thread just take the Context.getApplicationContext() which is a single global instance for the app. The application context is restricted in what you can do (no UI like Dialog for example) and requires some care (registered receivers and service bindings will be left forever if you don't clean them up), but is generally appropriate for code that isn't tied to a particular component's context. – Cori

That was incredibly helpful, thanks Dianne! I wish the documentation would be as good in the first place. – Enphytotic

You can't store mWorker in the activity that's going to be destroyed, but you can store it in some sort of persistent storage such as an Application subclass or some singleton. This still isn't perfect because those persistent storage solutions aren't really guaranteed to be persistent. Also, there's no guarantee that onDestroy() will be called. – Charentemaritime

@EdwardFalk: Yes, actually it depends on the task nature. Some actions just should not be implemented as AsyncTasks. They should be Services instead. – Ermey

I think that a simpler approach is just wrapping the task inside a fragment and then call setRetainInstace(true) in the onCreate method of this fragment. This is explained in the following article of the Alex Lockwood blog. – Berniecebernier

I find that the mActivity != null check is redundant here as the reference is not declared WeakReference - so activity here will not be GC'ed as long as the Worker object is alive. – Oleaginous

@Oleaginous : You are very right in saying that the activity here will not be GCed as long as the worker thread is alive. But, we set the reference to the activity as null when the activity is destroyed. After that, if we are trying to send out the progress inside the onProgressUpdate() method, that would lead to calling of a UI operation on the activity that has been killed. But, I still see this as flawed. How does this work if the activity does is in the background(e.g. user pressing on the home button)? – Bravin

I have a question regarding maintaining of the reference of the Activity inside the AsyncTask. What if I don't keep a reference but I only use MyActivity.this inside the AsyncTaskfor e.g. MyActivity.this.setProgressPercent(...). Can this be the solution against having a (weak)reference inside the AsyncTask? – Bravin

Answer 2

The reason is obvious: what if the activity gets destroyed which triggered the task?

Manually disassociate the activity from the AsyncTask in onDestroy(). Manually re-associate the new activity to the AsyncTask in onCreate(). This requires either a static inner class or a standard Java class, plus perhaps 10 lines of code.

Ania

Be careful with static references -- I have seen objects being garbage collected even though there were static strong references to them. Maybe a side-effect of Android's class loader, or a bug even, but static references are no safe way of exchanging state across an activity life-cycle. The app object is, however, that's why I use that. – Enphytotic

@Matthias: I did not say to use static references. I said to use a static inner class. There is a substantial difference, despite both having "static" in their names. – Ania

I would like to see this in code. Maybe someone can turn into code the idea. – Quemoy

@Pentium10: github.com/commonsguy/cw-android/tree/master/Rotation/… – Ania

I see -- they key here is getLastNonConfigurationInstance(), not the static inner class though. A static inner class keeps no implicit reference to its outer class so it's semantically equivalent to a plain public class. Just a warning: onRetainNonConfigurationInstance() is NOT guaranteed to be called when an activity is interrupted (an interruption can be a phone call, too), so you'd have to parcel your Task in onSaveInstanceState(), too, for a truly solid solution. But still, nice idea. – Enphytotic

Um... onRetainNonConfigurationInstance() is always called when the activity is in the process of being destroyed and re-created. It makes no sense to call at other times. If a switch to another activity happens, the current activity is paused/stopped, but it is not destroyed, so the async task can continue running and using the same activity instance. If it finishes and say displays a dialog, the dialog will correctly display as part of that activity and thus not show to the user until they return to the activity. You can't put AsyncTask in a Bundle. – Cori

@Matthias: as I understand, Android may restart the process, which will result in all static strong references being reset. – Streamlet

@Ania I'd love to see a version of that RotationAsync activity that does the same sort of thing with a CoroutineScope. i.e. nearly minimal code to run the most simple of tasks without having to create another layer for Fragments or ViewModels. I put together a question here: #59881967 – Bedrail

Answer 3

It looks like AsyncTask is a bit more than just conceptually flawed. It is also unusable by compatibility issues. The Android docs read:

When first introduced, AsyncTasks were executed serially on a single background thread. Starting with DONUT, this was changed to a pool of threads allowing multiple tasks to operate in parallel. Starting HONEYCOMB, tasks are back to being executed on a single thread to avoid common application errors caused by parallel execution. If you truly want parallel execution, you can use the executeOnExecutor(Executor, Params...) version of this method with THREAD_POOL_EXECUTOR; however, see commentary there for warnings on its use.

Both executeOnExecutor() and THREAD_POOL_EXECUTOR are Added in API level 11 (Android 3.0.x, HONEYCOMB).

This means that if you create two AsyncTasks to download two files, the 2nd download will not start until the first one finishes. If you chat via two servers, and the first server is down, you will not connect to the second one before the connection to the first one times out. (Unless you use the new API11 features, of course, but this will make your code incompatible with 2.x).

And if you want to target both 2.x and 3.0+, the stuff becomes really tricky.

In addition, the docs say:

Caution: Another problem you might encounter when using a worker thread is unexpected restarts in your activity due to a runtime configuration change (such as when the user changes the screen orientation), which may destroy your worker thread. To see how you can persist your task during one of these restarts and how to properly cancel the task when the activity is destroyed, see the source code for the Shelves sample application.

Streamlet

Answer 4

Probably we all, including Google, are misusing AsyncTask from the MVC point of view.

An Activity is a Controller, and the controller should not start operations that may outlive the View. That is, AsyncTasks should be used from Model, from a class that is not bound to the Activity life cycle -- remember that Activities are destroyed on rotation. (As to the View, you don't usually program classes derived from e.g. android.widget.Button, but you can. Usually, the only thing you do about the View is the xml.)

In other words, it is wrong to place AsyncTask derivatives in the methods of Activities. OTOH, if we must not use AsyncTasks in Activities, AsyncTask loses its attractiveness: it used to be advertised as a quick and easy fix.

Streamlet

Answer 5

I'm not sure it's true that you risk a memory leak with a reference to a context from an AsyncTask.

The usual way of implementing them is to create a new AsyncTask instance within the scope of one of the Activity's methods. So if the activity is destroyed, then once the AsyncTask completes won't it be unreachable and then eligible for garbage collection? So the reference to the activity won't matter because the AsyncTask itself won't hang around.

Greaten

true -- but what if the task blocks indefinitely? Tasks are meant to perform blocking operations, maybe even ones that never terminate. There you have your memory leak. – Enphytotic

Any worker that performs something in an endless loop, or anything that just locks up e.g. on an I/O operation. – Enphytotic

Answer 6

It would be more robust to keep a WeekReference on your activity :

public class WeakReferenceAsyncTaskTestActivity extends Activity {
    private static final int MAX_COUNT = 100;

    private ProgressBar progressBar;

    private AsyncTaskCounter mWorker;

    @SuppressWarnings("deprecation")
    @Override
    public void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_async_task_test);

        mWorker = (AsyncTaskCounter) getLastNonConfigurationInstance();
        if (mWorker != null) {
            mWorker.mActivity = new WeakReference<WeakReferenceAsyncTaskTestActivity>(this);
        }

        progressBar = (ProgressBar) findViewById(R.id.progressBar1);
        progressBar.setMax(MAX_COUNT);
    }

    @Override
    public boolean onCreateOptionsMenu(Menu menu) {
        getMenuInflater().inflate(R.menu.activity_async_task_test, menu);
        return true;
    }

    public void onStartButtonClick(View v) {
        startWork();
    }

    @Override
    public Object onRetainNonConfigurationInstance() {
        return mWorker;
    }

    @Override
    protected void onDestroy() {
        super.onDestroy();
        if (mWorker != null) {
            mWorker.mActivity = null;
        }
    }

    void startWork() {
        mWorker = new AsyncTaskCounter(this);
        mWorker.execute();
    }

    static class AsyncTaskCounter extends AsyncTask<Void, Integer, Void> {
        WeakReference<WeakReferenceAsyncTaskTestActivity> mActivity;

        AsyncTaskCounter(WeakReferenceAsyncTaskTestActivity activity) {
            mActivity = new WeakReference<WeakReferenceAsyncTaskTestActivity>(activity);
        }

        private static final int SLEEP_TIME = 200;

        @Override
        protected Void doInBackground(Void... params) {
            for (int i = 0; i < MAX_COUNT; i++) {
                try {
                    Thread.sleep(SLEEP_TIME);
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
                Log.d(getClass().getSimpleName(), "Progress value is " + i);
                Log.d(getClass().getSimpleName(), "getActivity is " + mActivity);
                Log.d(getClass().getSimpleName(), "this is " + this);

                publishProgress(i);
            }
            return null;
        }

        @Override
        protected void onProgressUpdate(Integer... values) {
            super.onProgressUpdate(values);
            if (mActivity != null) {
                mActivity.get().progressBar.setProgress(values[0]);
            }
        }
    }

}

Baronet

This is similar to what we first did with Droid-Fu. We would keep a map of weak references to context objects and would make a lookup in the task callbacks to obtain the most recent reference (if available) to run the callback against. Our approach however meant there was a single entity that maintained this mapping, whereas your approach doesn't, so this is indeed nicer. – Enphytotic

Did you have a look at RoboSpice ? github.com/octo-online/robospice. I do believe this system is even better. – Baronet

The sample code on the front page looks like it's leaking a context reference (an inner class keeps an implicit reference to the outer class.) Not convinced!! – Enphytotic

@Matthias, you are right, that's why I propose a static inner class that will hold a WeakReference on the Activity. – Baronet

@Matthias, did you speak about the front page of RoboSpice ? Where is there any leak ? – Baronet

"INNER CLASS of the activity, notified on UI Thread, if and only if your activity is alive". The code then continues to define a non-static inner class which references $this. Assuming this listener has to outlive activity restarts, it'll leak this reference to the outer activity. – Enphytotic

Thanks @Matthias, but you should really have a look at the API. Actually, this instance of the inner class won't create any memory leak as it will be garbage collected if the activity is destroyed (during onPause indeed). Our framework handles all that, and you can safely use an inner class, that is much more convenient to modify your activity when the request returns some result. We really handled all that in the framework. – Baronet

Oh so the listener is destroyed too when the activity is destroyed? I see, my bad--I was under the impression the listener is the piece that outlives the activity restart. – Enphytotic

@Matthias, no trouble, I realized this was not very clear in the docs, but we thought it would be quite an advanced topic. So we are the people to blame here :) But please Matthias, if you like this topic, we would love to hear from you, if you wanna give RoboSpice a try. – Baronet

it sure looks interesting, I was wondering though if the better way these days would be to go through the Loader classes (e.g. AsyncTaskLoader) which already handle these issues. I haven't worked with them in a production app yet, but I believe their premise is to remove the need to handle context attachment/detachment yourself. They still have other issues though, such as still not being able to raise exceptions in a task (which is a limitation of AsyncTask, not Loader.) – Enphytotic

@Matthias, I believe this starts being off topic. But loaders don't provide caching out of the box as we do, more over, loaders tend to be more verbose than our lib. Actually they handle pretty well cursors but for networking, a different approach, based on caching and a service is better suited. See neilgoodman.net/2011/12/26/… part 1 & 2 – Baronet

Answer 7

Why not just override the onPause() method in the owning Activity and cancel the AsyncTask from there?

Aleenaleetha

it depends on what that task is doing. if it just loads/reads some data, then it would be Ok. but if it changes the state of some data on a remote server then we'd prefer to give the task an ability to run to the end. – Ermey

@Arhimed and I take it if you hold up the UI thread in onPause that it's just as bad as holding it up anywhere else? I.e., you could get an ANR? – Aleenaleetha

exactly. we can not block the UI thread (be it a onPause or anithing else) because we risk to get an ANR. – Ermey

Answer 8

You are absolutely right - that is why a movement away from using async tasks/loaders in the activities to fetch data is gaining momentum. One of the new ways is to use a Volley framework that essentially provides a callback once the data is ready - much more consistent with MVC model. Volley was populised in the Google I/O 2013. Not sure why more people aren't aware of this.

Pozzy

thanks for that... i'm going to look into it... my reason for not liking AsyncTask is because it makes me stuck with one set of instructions onPostExecute...unless I hack it like using interfaces or overriding it every time I need it. – Unaneled

Answer 9

Personally, I just extend Thread and use a callback interface to update the UI. I could never get AsyncTask to work right without FC issues. I also use a non blocking queue to manage the execution pool.

Crackup

Well, your force close was probably because of the problem I mentioned: you tried referencing a context which had gone out of scope (i.e. its window had been destroyed), which will result in a framework exception. – Enphytotic

No... actually it was because the queue sucks that is built in to AsyncTask. I always use getApplicationContext(). I don't have problems with AsyncTask if it's only a few operations... but I am writing a media player that updates album art in the background... in my test I have 120 albums with no art... so, while my app didn't close all of the way, asynctask was throwing errors... so instead I built a singleton class with a queue that manages the processes and so far it works great. – Crackup

Answer 10

I thought cancel works but it doesn't.

here they RTFMing about it:

""If the task has already started, then the mayInterruptIfRunning parameter determines whether the thread executing this task should be interrupted in an attempt to stop the task."

That does not imply, however, that the thread is interruptible. That's a Java thing, not an AsyncTask thing."

http://groups.google.com/group/android-developers/browse_thread/thread/dcadb1bc7705f1bb/add136eb4949359d?show_docid=add136eb4949359d

Mclemore

Answer 11

You would be better off thinking of AsyncTask as something that is more tightly coupled with an Activity, Context, ContextWrapper, etc. It's more of a convenience when its scope is fully understood.

Ensure that you have a cancellation policy in your lifecycle so that it will eventually be garbage collected and no longer keeps a reference to your activity and it too can be garbage collected.

Without canceling your AsyncTask while traversing away from your Context you will run into memory leaks and NullPointerExceptions, if you simply need to provide feedback like a Toast a simple dialog then a singleton of your Application Context would help avoid the NPE issue.

AsyncTask isn't all bad but there's definitely a lot of magic going on that can lead to some unforeseen pitfalls.

Crackbrained

Answer 12

As to "experiences working with it": it is possible to kill the process along with all AsyncTasks, Android will re-create the activity stack so that the user will not mention anything.

Streamlet