AsyncTaskLoader onLoadFinished with a pending task and config change

Question

I'm trying to use an AsyncTaskLoader to load data in the background to populate a detail view in response to a list item being chosen. I've gotten it mostly working but I'm still having one issue. If I choose a second item in the list and then rotate the device before the load for the first selected item has completed, then the onLoadFinished() call is reporting to the activity being stopped rather than the new activity. This works fine when choosing just a single item and then rotating.

Here is the code I'm using. Activity:

public final class DemoActivity extends Activity
        implements NumberListFragment.RowTappedListener,
                   LoaderManager.LoaderCallbacks<String> {

    private static final AtomicInteger activityCounter = new AtomicInteger(0);

    private int myActivityId;

    private ResultFragment resultFragment;

    private Integer selectedNumber;

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

        myActivityId = activityCounter.incrementAndGet();
        Log.d("DemoActivity", "onCreate for " + myActivityId);

        setContentView(R.layout.demo);

        resultFragment = (ResultFragment) getFragmentManager().findFragmentById(R.id.result_fragment);

        getLoaderManager().initLoader(0, null, this);

    }

    @Override
    protected void onDestroy() {
        super.onDestroy();
        Log.d("DemoActivity", "onDestroy for " + myActivityId);
    }

    @Override
    public void onRowTapped(Integer number) {
        selectedNumber = number;
        resultFragment.setResultText("Fetching details for item " + number + "...");
        getLoaderManager().restartLoader(0, null, this);
    }

    @Override
    public Loader<String> onCreateLoader(int id, Bundle args) {
        return new ResultLoader(this, selectedNumber);
    }

    @Override
    public void onLoadFinished(Loader<String> loader, String data) {
        Log.d("DemoActivity", "onLoadFinished reporting to activity " + myActivityId);
        resultFragment.setResultText(data);
    }

    @Override
    public void onLoaderReset(Loader<String> loader) {

    }

    static final class ResultLoader extends AsyncTaskLoader<String> {

        private static final Random random = new Random();

        private final Integer number;

        private String result;

        ResultLoader(Context context, Integer number) {
            super(context);
            this.number = number;
        }

        @Override
        public String loadInBackground() {
            // Simulate expensive Web call
            try {
                Thread.sleep(5000);
            } catch (InterruptedException e) {
                e.printStackTrace();
            }

            return "Item " + number + " - Price: $" + random.nextInt(500) + ".00, Number in stock: " + random.nextInt(10000);
        }

        @Override
        public void deliverResult(String data) {
            if (isReset()) {
                // An async query came in while the loader is stopped
                return;
            }

            result = data;

            if (isStarted()) {
                super.deliverResult(data);
            }
        }

        @Override
        protected void onStartLoading() {
            if (result != null) {
                deliverResult(result);
            }

            // Only do a load if we have a source to load from
            if (number != null) {
                forceLoad();
            }
        }

        @Override
        protected void onStopLoading() {
            // Attempt to cancel the current load task if possible.
            cancelLoad();
        }

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

            // Ensure the loader is stopped
            onStopLoading();

            result = null;
        }

    }

}

List fragment:

public final class NumberListFragment extends ListFragment {

    interface RowTappedListener {

        void onRowTapped(Integer number);

    }

    private RowTappedListener rowTappedListener;

    @Override
    public void onAttach(Activity activity) {
        super.onAttach(activity);

        rowTappedListener = (RowTappedListener) activity;
    }

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

        ArrayAdapter<Integer> adapter = new ArrayAdapter<Integer>(getActivity(),
                                                                  R.layout.simple_list_item_1,
                                                                  Arrays.asList(1, 2, 3, 4, 5, 6));
        setListAdapter(adapter);

    }

    @Override
    public void onListItemClick(ListView l, View v, int position, long id) {
        ArrayAdapter<Integer> adapter = (ArrayAdapter<Integer>) getListAdapter();
        rowTappedListener.onRowTapped(adapter.getItem(position));
    }

}

Result fragment:

public final class ResultFragment extends Fragment {

    private TextView resultLabel;

    @Override
    public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) {
        View root = inflater.inflate(R.layout.result_fragment, container, false);

        resultLabel = (TextView) root.findViewById(R.id.result_label);
        if (savedInstanceState != null) {
            resultLabel.setText(savedInstanceState.getString("labelText", ""));
        }

        return root;
    }

    @Override
    public void onSaveInstanceState(Bundle outState) {
        super.onSaveInstanceState(outState);

        outState.putString("labelText", resultLabel.getText().toString());
    }

    void setResultText(String resultText) {
        resultLabel.setText(resultText);
    }

}

I've been able to get this working using plain AsyncTasks but I'm trying to learn more about Loaders since they handle the configuration changes automatically.

---

EDIT: I think I may have tracked down the issue by looking at the source for LoaderManager. When initLoader is called after the configuration change, the LoaderInfo object has its mCallbacks field updated with the new activity as the implementation of LoaderCallbacks, as I would expect.

public <D> Loader<D> initLoader(int id, Bundle args, LoaderManager.LoaderCallbacks<D> callback) {
    if (mCreatingLoader) {
        throw new IllegalStateException("Called while creating a loader");
    }

    LoaderInfo info = mLoaders.get(id);

    if (DEBUG) Log.v(TAG, "initLoader in " + this + ": args=" + args);

    if (info == null) {
        // Loader doesn't already exist; create.
        info = createAndInstallLoader(id, args,  (LoaderManager.LoaderCallbacks<Object>)callback);
        if (DEBUG) Log.v(TAG, "  Created new loader " + info);
    } else {
        if (DEBUG) Log.v(TAG, "  Re-using existing loader " + info);
        info.mCallbacks = (LoaderManager.LoaderCallbacks<Object>)callback;
    }

    if (info.mHaveData && mStarted) {
        // If the loader has already generated its data, report it now.
        info.callOnLoadFinished(info.mLoader, info.mData);
    }

    return (Loader<D>)info.mLoader;
}

However, when there is a pending loader, the main LoaderInfo object also has an mPendingLoader field with a reference to a LoaderCallbacks as well, and this object is never updated with the new activity in the mCallbacks field. I would expect to see the code look like this instead:

// This line was already there
info.mCallbacks = (LoaderManager.LoaderCallbacks<Object>)callback;
// This line is not currently there
info.mPendingLoader.mCallbacks = (LoaderManager.LoaderCallbacks<Object>)callback;

It appears to be because of this that the pending loader calls onLoadFinished on the old activity instance. If I breakpoint in this method and make the call that I feel is missing using the debugger, everything works as I expect.

The new question is: Have I found a bug, or is this the expected behavior?

Cr

Take a look at the CursorLoader.java source code. Try implementing onStartLoading, onStopLoading, onCanceled, onReset, and deliverResult similar to how the source code does... the LoaderManager assumes that all of these methods are implemented correctly. This might be why your implementation is only partially working across configuration changes. – Chantry

So it turns out that onLoadFinished() is actually being called - it's just reporting to the old activity (the one from before the configuration change) rather than to the new one. Question has been edited and the code has been updated. – Cr

Honestly, I think the problem here is the triviality of this example itself... in a real life situation the Loader wouldn't contain the actual data source (i.e. the private final int field is your actual data source, is it not?). Loaders are also supposed to monitor their data source and report back when changes are made. In most cases onLoadFinished won't be called on your new Activity after a configuration change since the Loader is smart enough to retain its old data... it will only re-load if it sees that changes to its backing data has been made. – Chantry

Sorry, what I meant to say is onLoadFinished will be called... but it will simply retain the old data instead of re-loading new data (which can be an expensive and often unnecessary operation). – Chantry

What integer values display in the Activity before and after these situations? – Chantry

For the sake of this example we can pretend that the integers in the list are product IDs for a server call that returns the details for each product. Does that help make the example less trivial? – Cr

But shouldn't you store that data in a database or on disk somehow? The problem could very well be that you are attempting to store this data within the Loader itself. – Chantry

I'm not attempting to store anything within the Loader though - I'm just trying to use the Loader to load the data from the server. It doesn't need to be persisted. – Cr

So is the problem just that onLoadFinished is not called or is the int showing an incorrect value? – Chantry

Updated question to include new information gained from reading the source code to LoaderManager. The code example has also been updated to simulate a more real-world scenario of loading product details as each product ID is chosen. – Cr

I've written a blog post on Loaders recently... maybe you will find it helpful :) androiddesignpatterns.com/2012/08/implementing-loaders.html – Chantry

@AlexLockwood The blog post is helpful for general understanding of Loaders (I wish it would have been around a few months ago), but doesn't do anything toward helping understand or resolve this particular issue. I'm planning to file a bug for this issue soon and hopefully get some sort of resolution one way or the other. – Cr

Submitted issue at code.google.com/p/android/issues/detail?id=36778 – Cr

Did anyone ever find a solution for this problem? Enqueuing as new pending loader happens, but then when deliverResult() is called, Loader#mListener is null, so the results are never returned to my callback. – Chaing

Answer 1

In most cases you should just ignore such reports if Activity is already destroyed.

public void onLoadFinished(Loader<String> loader, String data) {
    Log.d("DemoActivity", "onLoadFinished reporting to activity " + myActivityId);
    if (isDestroyed()) {
       Log.i("DemoActivity", "Activity already destroyed, report ignored: " + data);
       return;
    }
    resultFragment.setResultText(data);
}

Also you should insert checking isDestroyed() in any inner classes. Runnable - is the most used case.

For example:

// UI thread
final Handler handler = new Handler();
Executor someExecutorService = ... ;
someExecutorService.execute(new Runnable() {
    public void run() {
        // some heavy operations
        ...
        // notification to UI thread
        handler.post(new Runnable() {
            // this runnable can link to 'dead' activity or any outer instance
            if (isDestroyed()) {
                return;
            }

            // we are alive
            onSomeHeavyOperationFinished();
        });
    }
});

But in such cases the best way is to avoid passing strong reference on Activity to another thread (AsynkTask, Loader, Executor, etc).

The most reliable solution is here:

// BackgroundExecutor.java
public class BackgroundExecutor {
    private static final Executor instance = Executors.newSingleThreadExecutor();

    public static void execute(Runnable command) {
        instance.execute(command);
    }
}

// MyActivity.java
public class MyActivity extends Activity {
    // Some callback method from any button you want
    public void onSomeButtonClicked() {
        // Show toast or progress bar if needed

        // Start your heavy operation
        BackgroundExecutor.execute(new SomeHeavyOperation(this));
    }

    public void onSomeHeavyOperationFinished() {
        if (isDestroyed()) {
            return;
        }

        // Hide progress bar, update UI
    }
}

// SomeHeavyOperation.java
public class SomeHeavyOperation implements Runnable {
    private final WeakReference<MyActivity> ref;

    public SomeHeavyOperation(MyActivity owner) {
        // Unlike inner class we do not store strong reference to Activity here
        this.ref = new WeakReference<MyActivity>(owner);
    }

    public void run() {
        // Perform your heavy operation
        // ...
        // Done!

        // It's time to notify Activity
        final MyActivity owner = ref.get();
        // Already died reference
        if (owner == null) return;

        // Perform notification in UI thread
        owner.runOnUiThread(new Runnable() {
            public void run() {
                owner.onSomeHeavyOperationFinished();
            }
        });
    }
}

Impresario

Answer 2

Maybe not best solution but ... This code restart loader every time, which is bad but only work around that works - if you want to used loader.

Loader l = getLoaderManager().getLoader(MY_LOADER);
if (l != null) {
    getLoaderManager().restartLoader(MY_LOADER, null, this);
} else {
    getLoaderManager().initLoader(MY_LOADER, null, this);
}

BTW. I am using Cursorloader ...

Klement

Answer 3

A possible solution is to start the AsyncTask in a custom singleton object and access the onFinished() result from the singleton within your Activity. Every time you rotate your screen, go onPause() or onResume(), the latest result will be used/accessed. If you still don't have a result in your singleton object, you know it is still busy or that you can relaunch the task.

Another approach is to work with a service bus like Otto, or to work with a Service.

Homeland

Answer 4

Ok I'm trying to understand this excuse me if I misunderstood anything, but you are losing references to something when the device rotates.

Taking a stab...

would adding

android:configChanges="orientation|keyboardHidden|screenSize"

in your manifest for that activity fix your error? or prevent onLoadFinished() from saying the activity stopped?

Farreaching

That may be a workaround, but this is most likely a genuine bug in Android. – Cankerworm

@StevePomeroy not necessarily, when the device is rotated; the activity is recreated, thus re-initializing any references to objects or other variables of the sort. This is preventing the system from re-creating the activity by passing it to the activity itself allowing it to handle/override to orientation change. Some layout elements might be re-sized or re-positioned if the device went into landscape mode. – Farreaching

@StevePomeroy Your asyncloader never truly finishes since its being disposed of on the destruction and reconstruction of the activity – Farreaching

take a look at the bug listed above ( code.google.com/p/android/issues/detail?id=36778 ) and try out the example code. The behaviour is very unusual and is most certainly not as desired (unless it's intended to not function). – Cankerworm

Also: you are right - handling the config changes yourself is one way of solving the issue. But what if Android decides to clean up your activity for some reason that's not handled by the config changes call? It's still important to fix the bug that appears when not handling config changes yourself. – Cankerworm