Background image loader AsyncTask

Question

Usually, AsyncTasks are meant to be one-shot, i.e. you start it, it fetches some data, displays the result and dies. However, I'm now using a long-running AsyncTask to load images as the need arises. So far it's working great, but I'd just like to know if this is good practice & if there's any threading detail that I missed. Here's my slimmed down implementation:

public class ImageLoader extends AsyncTask<Void, Object, Void> {
 private final List<String> pendingURLs = new Vector<String>();
 private Thread backgroundThread;
 @Override
 protected Void doInBackground(Void... params) {
 this.backgroundThread = Thread.currentThread();
 while (!backgroundThread.isInterrupted()) {
 String url = null;
 synchronized(this) {
 if (pendingURLs.size() > 0) {
 url = pendingURLs.remove(0);
 }
 }
 if (url != null) {
 // fetch image...
 publishProgress(url, fetchedBitmap); // in onProgressUpdate I call a listener that can be implemented by any Fragment or other component
 }
 try {
 synchronized(this) {
 if (pendingURLs.size() == 0) {
 wait();
 }
 }
 } catch (InterruptedException e) {
 break;
 }
 }
 }
 public syncrhonized void addUrl(String url) {
 pendingURLs.add(url);
 notify();
 }
 public Thread getBackgroundThread() {
 return backgroundThread;
 }
}

Then, in my Fragment#onCreate() I create a new instance and execute it like this:

mLoader = new ImageLoader();
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.HONEYCOMB) {
 mLoader.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
} else {
 mLoader.execute();
}

Finally, in my Fragment#onDestroy() I do:

if (mLoader != null && mLoader.getBackgroundThread() != null) {
 mLoader.getBackgroundThread().interrupt(); // this will cause it to break out of the while loop and finish the thread
}

Answer 1

For starters, you should replace your Vector with an ArrayBlockingQueue . Then you can drop the wait and the null check. Can also then remove synchronization on the add method since its handled by the data structure.

private final BlockingQueue<String> pendingURLs = new ArrayBlockingQueue<String>(64);
protected void doInBackground(Void... params) {
 this.backgroundThread = Thread.currentThread();
 while (!backgroundThread.isInterrupted()) {
 String url = pendingURLs.take();
 // fetch image...
 publishProgress(url, fetchedBitmap); // in onProgressUpdate I call a listener that can be implemented by any Fragment or other component
 }
public void addUrl(String url) {
 pendingURLs.put(url);
}

You should probably replace it with just a new thread instead of using asynctask at all.

public class ImageLoader implements Runnable {
 private final BlockingQueue<String> pendingURLs = new ArrayBlockingQueue<String>();
 public void run() {
 while (true) {
 String url = pendingURLs.take(); 
 // fetch image...
 publishProgress(url, fetchedBitmap); // in onProgressUpdate I call a listener that can be implemented by any Fragment or other component
 }
 }
 public void addUrl(String url) {
 pendingURLs.put(url);
 }
}

Then you can create it in your initialization somewhere, ie

 ImageLoader loader = new ImageLoader();
 new Thread(loader, "BackgroundLoader").start();

• Felix: +1 Nice improvement to use the blocking queue. However, I wouldn't stray from AsyncTask since it handles calling onProgressUpdate on the UI thread whenever I call publishProgress from the background thread.
• Namrata: which one is better, Android loader or AsyncTask ????