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

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.

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(...);
    }
}

> 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.

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.

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.

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.

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]);
			}
		}
	}

}

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

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.

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.

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

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.

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.