How to correctly manage locks with AsyncTask

Question

I have an android application where I need to repeatedly get a JSON from a server (which may fail), display the information, and allow the user the change the address of the server. It seemed like the example here would do the trick since I could asynchronously attempt to get my JSON in the doInBackground and just call runAgain() in onProgressUpdate based on a timer, and if the user changes the server address, I can call terminateTask() and start a new GetDataFromServerTask. However, when I call runAgain() in onProgressUpdate, I always get IllegalMonitorStateException. And if check isHeldByCurrentThread() in onProgressUpdate, the result is false. So can anyone expand the example to identify why I don't seem to get a lock? Or if there is a better method to accomplish my task feel free to propose that instead. Thanks!

EDIT: Added complete code using the above reference. (This version is only for testing and does not do everything described above.)

package com.testing.asynctest;

import android.app.Activity;
import android.os.AsyncTask;
import android.os.Bundle;
import android.util.Log;
import java.util.concurrent.locks.Condition;
import java.util.concurrent.locks.ReentrantLock;

public class MyActivity extends Activity {

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

        new GetDataFromServerTask().execute();
    }

    class GetDataFromServerTask extends AsyncTask<Void, Void, Void> {

        private final ReentrantLock lock = new ReentrantLock();
        private final Condition tryAgain = lock.newCondition();
        private volatile boolean finished = false;

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

            try {
                lock.lockInterruptibly();
                try {
                    do {
                        // This is the bulk of our task, request the data, and put in "result"
                        Log.d("MyActivity", "Perform Main Task");

                        // Return it to the activity thread using publishProgress()
                        publishProgress();

                        // At the end, we acquire a lock that will delay
                        // the next execution until runAgain() is called..
                        Log.d("MyActivity", "Acquiring Lock");
                        tryAgain.await();
                        Log.d("MyActivity", "Lock was released");

                    } while(!finished);

                } finally {
                    lock.unlock();
                }
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
            return null;
        }

        @Override
        protected void onProgressUpdate(Void... result)
        {
            // Treat this like onPostExecute(), do something with result

            // This is an example...
            runAgain();
        }

        public void runAgain() {
            // Call this to request data from the server again
            if ( lock.isHeldByCurrentThread() ) {
                Log.d("MyActivity", "Releasing Lock");
                tryAgain.signal();
            } else {
                Log.d("MyActivity", "Lock Not Held by Thread");
            }
        }

        public void terminateTask() {
            // The task will only finish when we call this method
            finished = true;
            lock.unlock();
        }

        @Override
        protected void onCancelled() {
            // Make sure we clean up if the task is killed
            terminateTask();
        }
    }
}

The LOG output shows that I do not have the Lock that I expect to have.

 com.testing.asynctest D/MyActivity﹕ Perform Main Task
 com.testing.asynctest D/MyActivity﹕ Acquiring Lock
 com.testing.asynctest D/MyActivity﹕ Lock Not Held by Thread

I understand the meaning of IllegalMonitorStateException, and have tested with isHeldByCurrentThread to verify that I don't hold the lock. I just don't understand what is wrong with this example resulting in me not having the lock. Thanks!

I'm trying to follow the example in the [link](http://stackoverflow.com/a/6374135/3571110). The link describes the approach. In short, the do loop is the repeating the task. The lock pauses the repeat until runAgain() is called. My plan is to wrap runAgain() in a timer that would fire every 30 seconds. I have not implemented the timer yet since this simpler case does not work. — proximous, Aug 05 '14 at 17:48
why it throws you IllegalMonitorStateException im not sure. But lock.isHeldByCurrentThread() normally should return false because it is called from the main thread. Debug with getWaitingThreads() to get a better picture of whats going on. — eldjon, Aug 05 '14 at 18:02
Thanks for the clarification on isHeldByCurrentThread(). Can you point me to an example on how to use getWaitingThreads() to debug? My feeble attempts to use it are resulting in IllegalMonitorStateException at my call to getWaitingThreads(). — proximous, Aug 05 '14 at 18:37
if it throws IllegalMonitorState exception it means for sure it is not being held by any thread. In order to debug just print sth everywhere the unlock/signal is called. Meanwhile i may suggest take a look at Handler and HandlerThread. Seems a better approach as the intent of AsyncTask is for short lasting operations. I can provide with a sample code if you are not familiar — eldjon, Aug 05 '14 at 18:55

score 0 · Accepted Answer · answered Aug 05 '14 at 04:09

0

Per documentation on IllegalMonitorStateException:

Thrown when a monitor operation is attempted when the monitor is not in the correct state, for example when a thread attempts to exit a monitor which it does not own.

You haven't provided enough info (i.e. relevant code), but I'd guess that you are trying to wait()/notify() on an Object that you do not held the lock.

answered Aug 05 '14 at 04:09

Kai

15,284
6
51
82

I'm using the code in the example link I provided. I can repaste it here if that helps. – proximous Aug 05 '14 at 16:59
Looking at the code, I am not sure that it would behave correctly under all circumstances, and threading related issues is a real possibility. I would suggest you keep it simple and only use these kinds of fancy solution when its necessity has been proved. – Kai Aug 05 '14 at 17:08
Can you suggest a better approach then? The JSON "get" can timeout or fail, so I think I want to use an AsyncTask to get the JSON. The two complications I have are (1) wanting automatic refresh (get the JSON every 30 seconds) and (2) the ability for the user to select a different server. The sample code seemed like the best/easiest way to accomplish that. – proximous Aug 05 '14 at 17:38
Just use vanilla AsyncTask – Kai Aug 06 '14 at 02:40
Although I'd still like to know why this approach doesn't work, I accept there are other ways to solve this and have moved on to a different approach. – proximous Sep 02 '14 at 19:48

How to correctly manage locks with AsyncTask

1 Answers1