Android runOnUiThread thread safety

Question

I was wondering if my approach to a specific problem is thread-safe (probably not, that's why I'm asking). Suppose we have this code, running on a non-UI thread:

if (myMap != null)
{
    runOnUiThread(new Runnable()
    {
        @Override
        public void run()
        {
           Something something = myMap.get(someKey);
           // update some views and stuff                                       
        }
    });
}

I'm guessing I have no guarantee that myMap will be valid by the time the runnable actually executes, right? If that is the case, should I move the myMap != null inside the run() method or just duplicate it there? What would be the best approach if I want to ensure that the runOnUiThread code is executed only if the map is not null?

Thanks

run the this code after you will initialize myMap Map, so it won't be null. — Android Killer, Aug 05 '13 at 10:49
From what thread are you calling that? If it is called from the UiThread, it will be executed directly inline as if there was no `Runnable`. — zapl, Aug 05 '13 at 11:01
I forgot to mention that the code is not being run from the UI thread. Gonna edit that in. — npace, Aug 05 '13 at 11:18

score 2 · Answer 1 · answered Aug 05 '13 at 10:50

2

This is as simple as defining myMap before executing the code you have given:

    Map<String, String> myMap = new Hashmap<String, String>();
    //if (myMap != null)
   // {
        runOnUiThread(new Runnable()
        {
            @Override
            public void run()
            {
               Something something = myMap.get(someKey); //myMap won't be null
               // update some views and stuff                                       
            }
        });
   // }

If you are trying to check if any data is inside then u can do using size() method

if(myMap.size() != 0) {
//do your stuffs
}

answered Aug 05 '13 at 10:50

Android Killer

18,174
13
67
90

myMap is actually a reference to a map I'm getting from a method call. But that doesn't matter - before I get it via the method, I now construct a new HashMap of initial capacity 0, as per your advice. I hope it works as there is no easy way to recreate this problem... – npace Aug 05 '13 at 11:23
`myMap` should be final to avoid http://stackoverflow.com/questions/16062274/how-can-an-immutable-object-with-non-final-fields-be-thread-unsafe – zapl Aug 05 '13 at 11:32

score 1 · Answer 2 · answered Aug 05 '13 at 11:04

When you create a Runnable and pass it to the function "runOnUiThread()" if the current thread is main thread it will be executed immediately or will be put into the queue and will be executed later and that time the object can be not valid. It would be better if you check its validity in the worker thread (in run() method of Runnable).

zapl · Accepted Answer · 2013-08-05T11:51:08.790

Your general structure is not thread-safe

The thread that initiates the whole process checks myMap and initiates execution in potentially another thread.

if (myMap != null) {
    doSomethingInAnotherThread();
}
// something can set `myMap` to null here...

The code that is scheduled to run will at some point do

void inAnotherThread() {
    myMap.access();
}

but there is no more guarantee that myMap is still the same as it was before. If there are threads that can change what myMap refers to then doing

void inAnotherThread() {
    if (myMap != null) {
        myMap.acess();
    }
}

would still not be thread safe since you access myMap twice and it can be different each time. E.g. It is not null inside the if but null once you access it. One solution is to copy the reference so nothing can change your local copy of that reference while you work with it.

void inAnotherThread() {
    Map localReference = myMap;
    if (localReference != null) {
        localReference.acess();
    }
}

Whether that is threadsafe or not depends on myMap. For one whether it is volatile (or final) or not. If it is: other threads are guaranteed to see the most recent version of what myMap refers to, if not: nothing guaranteed. (Note: I think runOnUiThread established a happens-before relation since it synchronizes internally and you should therefore have some sort of guarantee to see a recent version of the reference)

The next point once you have a reference to the correct Map instance is what you can do with it safely. A simple HashMap is not threadsafe to use. If you call .get() it could still blow up on you if - at the same time - another thread calls put/remove/.. and therefore changes data while .get is accessing it.

You could wrap it in Collections.synchronizedMap(map) which would make single operations like get atomic so other threads can't interfere. But it's still not threadsafe for everything. E.g. iterating over the values will still fail if you don't synchronize externally. That problem could be solved by using a ConcurrentHashMap which supports iteration as well.

Threadsafety depend on a lot of factors and on your definition of what thread safe is. What you wrote is already threadsafe if you can guarantee that myMap is never changed once it is set to != null and you know that the background thread is done modifying myMap so it is safe for the UiThread to access it.

Thanks for this extensive post, it gave me some food for thought. After further examination of the code I'm working on (not my own code but legacy), it turned out that the global map reference that was causing my problems is redundant so I removed it altogether. — npace, Aug 05 '13 at 13:25

Android runOnUiThread thread safety

3 Answers3