Thread safe access to arraylist: two producer one consumer. Lock object is enough?

Question

I have two thread that can produce value and add it in a arraylist, and other thread can access to it to read a value.

My problem is that the producer can access to the list in the same time that the consumer use data.

This is my code :

 public class CommandTree
 {  

Lock lock = new ReentrantLock();

ArrayList<Command> cmdToSend = null;
JSONObject sendCmdMap;

public CommandTree(JSONObject sendCmdMap)
{
    this.cmdToSend = new ArrayList<Command>();
    this.sendCmdMap = sendCmdMap;
}

private synchronized void addMacroCmd(String macro, int fmt, int tgt, int sid,int count,JSONArray sli,String paramName,JSONObject params,int function)
{

    boolean check = false;
    int i = 0;

    lock.lock();
        try
        {
            for(i=0; i<cmdToSend.size(); i++)
            {
                if(cmdToSend.get(i).getMacroName().equalsIgnoreCase(macro))
                {
                    check = true;
                    break;
                }
            }

            if(check == false)
            {
                cmdToSend.add(new Command(macro,fmt,tgt,sid,count,function,sli));
            }

            if(paramName != null)
            {
                if(check)
                    cmdToSend.get(i).setParameter(paramName,params);
                else
                    cmdToSend.get(cmdToSend.size()-1).setParameter(paramName,params);
            }
        }
        finally
        {
            lock.unlock();
        }
}


private void addParameter(String macro,int fmt, int tgt, int sid,int count,JSONArray sli,String paramName,JSONObject params,int function)
{
    lock.lock();
    try
    {
        this.addMacroCmd(macro, fmt, tgt, sid, count,sli, paramName,params,function);
    }
    finally
    {
        lock.unlock();
    }

}

public int getSize()
{
    return cmdToSend.size();
}

public void reset()
{
    lock.lock();
    try
    {
        cmdToSend.clear();
    }
    finally
    {
        lock.unlock();
    }

}

/*
public Command getNextCommandInLoop()
{
    return cmdToSend.;
}
*/
public Command getNextCommand(int i)
{
    Command result;

    lock.lock();
    try
    {
        result = cmdToSend.get(i);
    }
    finally
    {
        lock.unlock();
    }

    return result;
}

public synchronized boolean populateCommandTree(String i,String target) throws JSONException
{
    JSONObject tgtCmd = (JSONObject) sendCmdMap.get(target);
    JSONObject cmdObject;

    Iterator<String> iter = tgtCmd.keys();

    while (iter.hasNext()) 
    {
        String key = iter.next();

        if(key.equalsIgnoreCase(i))
        {
            //it is a general commands
            JSONObject macro = (JSONObject)tgtCmd.opt(key);

            cmdObject = (JSONObject) macro.opt("cmd");


            addMacroCmd(key,cmdObject.optInt("fmt"),cmdObject.optInt("tgt"),cmdObject.optInt("sid"),cmdObject.optInt("count"),cmdObject.optJSONArray("sli"),null,null,macro.optInt("function"));

            return true;
        }   
        else
        {
            //It is a parameter, we have to search its general command

            cmdObject = (JSONObject)tgtCmd.opt(key);

            if(cmdObject == null)
            {
                continue;
            }

            JSONObject parameter = cmdObject.optJSONObject("Parameter");

            if( parameter == null)
            {
                //There isn't the requested command, we iterate on the next one
                continue;
            }
            else
            {

                if(((JSONObject) parameter).optJSONObject(i) != null)
                {
                    JSONObject cmdStructure = (JSONObject)cmdObject.opt("cmd");
                    //We have found the  command, save it in commandSendCache
                    addMacroCmd(key,cmdStructure.optInt("fmt"),cmdStructure.optInt("tgt"),cmdStructure.optInt("sid"),cmdStructure.optInt("count"),cmdStructure.optJSONArray("sli"),i,parameter.optJSONObject(i),cmdObject.optInt("function"));
                    return true;//(JSONObject)tgtCmd.opt(key);
                }
                else
                {
                    continue;
                }
            }
        }
    }

    return false;
}}

I read some post on that case, but I don't understand very well. I thought to post my code in this way I can understand in better way.

Other problem is that one producer is a UI thread, and I worried if there is problem to stop the UI thread for some times.

I also thought to use ConcurrentLinkedQueue because some time I need to loop on the list, and I always extract the value from the first position, but with concurrentLInkedQueue I don't know how can implementate the loop and in what way I can implementate the addMacroCmd method..

In my case I think to use lock object and ArrayList.

Do you have some suggestion ? I want to learn in better way the concurrency, but it not very easy for me :(

EDIT : the following is the part of code that add and remove the command :

public synchronized void readSensorData(String[] sensor, String target)
    {
        cmdTree.reset(); 

        for(int i=0;i<sensor.length;i++)
        {
            try 
            {
                cmdTree.populateCommandTree(sensor[i],target);
            } 
            catch (JSONException e) 
            {

            }
        }

        writeExecutor.execute(this.writeCommandTree);

    }




    /**
     * 
     * @param i
     * @param target
     * @return
     * @throws JSONException when the command requested doesn't exists
     */



    private ByteArrayOutputStream f = new ByteArrayOutputStream();
    ExecutorService writeExecutor = Executors.newSingleThreadExecutor();

    Semaphore mutex = new Semaphore(0);

    volatile boolean diagnostic = false;
    volatile int index = 0;

    Runnable writeCommandTree = new Runnable()
    {

        @Override
        public void run() 
        {
            while(index < cmdTree.getSize())
            {
                writeCmd();
                try 
                {
                    mutex.acquire();
                } 
                catch (InterruptedException e) 
                {
                    e.printStackTrace();
                }

            }

            sendAnswerBroadcast("answer", answer); 
            answer = new JSONObject();
            index = 0;

        }

    };

and the mutex is release when arrive a new response .

Addictional information :

The readSensorData() is called when button on the ux (UI Thread) is pressed and in same case from other Thread B. WriteCommandTree is only execute in the executor (Other Thread C).

I change the name of getnextcommand into getcommand - getcommand(int i) is called in the callback of the response (sometime is in other thread (i'm forget to that function ...) and in writecmd inside writecommandtree - getsize in the writecommandTree in the thread C

Answer 1

Don't get headaches just for synchronizing a list, simply use the Java standard library :

List<Command> commands = Collections.synchronizedList(new ArrayList<>());

By the way, a naive implementation of this would simply to wrap an unsafe list and add synchronized to all the methods.

Answer 2

You can use blockingQueue to achieve the same. Refer simple tutorial about blockingQueue :http://tutorials.jenkov.com/java-util-concurrent/blockingqueue.html

Answer 3

There are several problems with this code:

• It is unlikely that you need both a ReentrantLock and synchronization.
• The getSize method is not synchronized at all. If, e.g., reset is called from a thread other than the one from which getSize is called, the program is incorrect.
• sendCmdMap is leaked in CommandTree's constructor. If the thread that creates the CommandTree is different from the thread that calls populateCommandTree, the program is incorrect.

Note, btw, that using a synchronized view of cmdToSend would not fix any of these problems.

What you need to do, here, is this:

• Producers need to seize a lock, hand a command to the CommandTree and then delete all references to it.

• Consumers need to seize the same lock and get a reference to a command, deleting it from the CommandTree.

For problems like this, there is no better reference than "Java Concurrency in Practice"