Synchronized List - add multiple threads and remove one thread-CodePudding

I have this code:

private static boolean flagStop = false;

//synchronized list
private static List<Object> queue = Collections.synchronizedList(new ArrayList<>());

//thread which check once per second the size of queue
//and print elements from list and try to remove them safely
private static Thread printQueueThread = new Thread(() -> {
    while (!flagStop) {
        if (!queue.isEmpty()) {
            int size = queue.size();
            List<Object> copyList = queue.subList(0, size);
            for (int i = 0; i < size; i  ) {
                queue.remove(0);
            }
            copyList.forEach(System.out::println);
        }
        try {
            Thread.sleep(1_000);
        }
        catch (Exception ignored) { }
    }
});

static {
    printQueueThread.start();
}

//add elements in queue
//this is accessed by multiple threads, by few times per second.
public static void addElement(Object obj) {
    queue.add(obj);
}

In this class, I have a synchronized list which I'm using to add elements from different threads, and in another thread print and remove every element.

My point is to not stop the threads at addElement and to process(print and remove) the elements safely.

I'm posting here to ask if my code is thread safe or how to check it.

CodePudding user response：

It appears to me that List is not the class you're looking for. Java actually has a Queue class (and a thread safe implementation of it).

Then, looking further into your code, I can see that it's very wasteful; you're creating a copy of the List, removing the object from the original and then printing the copy. That seems like a CopyOnWriteArrayList but less thread safe to me. This can be useful, but the Queue solution would be best suitable in this case and more performant.

This is what your code would look like after changing it to use BlockingQueue:

private static boolean flagStop = false;

//synchronized list
private static Queue<Object> queue = new BlockingQueue<Object>();

//thread which check once per second the size of queue
//and print elements from list and try to remove them safely
private static Thread printQueueThread = new Thread(() -> {
    Object currentObject;
    while(!flagStop) {
            while((object = queue.poll()) != null) {
                System.out.println(currentObject);
            }
        }
        try { 
            Thread.sleep(1_000); 
        } catch (Exception ignored) {
        }
    }
});

static{
    printQueueThread.start();
}

//add elements in queue
//this is accessed by multiple threads, by few times per second.
public static void addElement(Object obj){
    queue.offer(obj);
}

On the other hand, you're also using a boolean to tell your thread whether it should stop or not. That can cause synchronization issues. You should really replace it with AtomicBoolean instead - a thread safe implementation of boolean. Changing boolean flagStop = false to AtomicBoolean flagStop = new AtomicBoolean() and while (!flagStop) to while(!flagStop.get()).

I'm not really sure as to why your class is also all static, but that's generally bad practice. I would avoid that and use instances. I would also suggest implementing Runnable, making the code more clear and adding a null check in the addElement. With all the corrections your final class would look like this:

public class MyPrintQueue implements Runnable {
    private AtomicBoolean shouldStop;

    //synchronized list
    private Queue<Object> queue;
    
    //thread which check once per second the size of queue
    //and print elements from list and try to remove them safely
    private Thread printQueueThread;
    
    public MyPrintQueue() {
        this.queue = new BlockingQueue<>();
        this.printQueueThread = new Thread(this);
    }

    @Override
    public void run() {
        Object currentObject;
        while(!flagStop.get()) {
                while((object = queue.poll()) != null) {
                    System.out.println(currentObject);
                }
            }
            try { 
                Thread.sleep(1_000); 
            } catch (Exception ignored) {
                // Ignored
            }
        }
    }

    public void startThread() {
        printQueueThread.start();
    }

    public void stop() {
        flagStop.set(true);
    }

    //add elements in queue
    //this is accessed by multiple threads, by few times per second.
    public void addElement(Object obj){
        if (obj == null)
            return;
        queue.offer(obj);
    }
}

That being said, your code doesn't seem completely thread safe to me. I might be wrong, but you could remove the Thread.sleep call and create a TestClass which calls addElement() in a while true to check if any exception is ever thrown. If you don't want to use Queue and must use a List, you should then replace Collections.synchronizedList with a CopyOnWriteArrayList<>(), effectively changing your code to this

private static boolean flagStop = false;

//synchronized list
private static List<Object> queue = new CopyOnWriteArrayList<>();

//thread which check once per second the size of queue
//and print elements from list and try to remove them safely
private static Thread printQueueThread = new Thread(() -> {

    while(!flagStop){

        if(!queue.isEmpty()){
            while (queue.size() > 0) {
                System.out.println(queue.get(0));
                queue.remove(0);
            }
        }

        try{ Thread.sleep(1_000); }catch(Exception ignored){}

    }

});

static{
    printQueueThread.start();
}

//add elements in queue
//this is accessed by multiple threads, by few times per second.
public static void addElement(Object obj){
    queue.add(obj);
}

Note that the code above is still static (!) and does not implement AtomicBoolean. Also take a look at the following question and its answers which seem to be related to yours: Is externally synchronized ArrayList thread safe if its fields are not volatile?