Java Multithreading - Remove items from list

Question

new to multithreading here please bear with me. I'm trying to run 2 threads which remove items from a list of tasks (10 tasks in total) until the taskList is empty.

What i have so far is:

Main method:

public static void main(String[] args) {


        List<Task> taskList = new ArrayList<Task>();
        List<Thread> threadList = new ArrayList<Thread>();


        for (int i = 1; i <= 10; i++) {

            taskList.add(new Task("some details");
        }

        TaskManager manager = new TaskManager();
        gestor.setTaskList(taskList);

        Thread t1 = new Thread(taskManager);            
        Thread t2 = new Thread(taskManager);    

        threadList.add(t1);
        threadList.add(t2);

        if(threadList.size() > 0){
            for (Thread thread : threadList){                               
                thread.start();             
            }
        }   

        for (Thread thread : threadList){
            try {
                thread.join();
            } catch (InterruptedException e) {
                System.out.println("thread " + Thread.currentThread().getName() + " was interrupted");
            }
        }

        System.out.println("END OF MAIN");

    }

Task Manager class:

public class TaskManager implements Runnable {

    private List<Task> availableTasks;
    private Random random = new Random();

    public void setAvailableTasks(List<Task> availableTasks) {
    this.availableTasks = availableTasks;
}

    @Override
    public void run() {
        while (!availableTasks.isEmpty()) {
            takeTask();
        }
    }

    public void takeTask() {
        try {
            Thread.sleep(1000);
            int index = random.nextInt(availableTasks.size());
            Task task = availableTasks.get(index);

            printDetails(task);
            availableTasks.remove(task);
        } catch (InterruptedException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }
    }

    public void printDetails(Task task) {
        //here it should print the details to the console  
    }
}

The thing is, it either runs 2 times or it's always the same thread running 10 times. I know it's probably a silly question but I hope someone can clarify it! Thanks in advance

Edit: I was able to make it work using @Lidae's suggestion

Take task method edited like so:

public void takeTask() {
        try {
            Thread.sleep(1000);
            synchronized (this) {
                if (!availableTasks.isEmpty()) {
                    int index = random.nextInt(availableTasks.size());
                    Task task = availableTasks.get(index);

                    printDetails(task);
                    availableTasks.remove(task);
                }
            }
        } catch (InterruptedException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }

where are you creating and starting multiple threads?

uylmz
– uylmz

2016-12-10 20:18:17 +00:00
Commented Dec 10, 2016 at 20:18 — uylmz
– uylmz, Commented Dec 10, 2016 at 20:18
edited with main method

Juan M
– Juan M

2016-12-10 20:30:26 +00:00
Commented Dec 10, 2016 at 20:30 — Juan M
– Juan M, Commented Dec 10, 2016 at 20:30

Lidae · Accepted Answer · 2016-12-10 20:54:42Z

2

Your code has some concurrency problems as a result of several threads trying to access the same object at the same time. For example one thing that can happen is that thread A gets a task from the list (in Task task = availableTasks.get(index)), then there is a context switch and that very task is removed by thread B, and by the time thread A tries to remove the task, it is already gone (this wouldn't cause an exception in your code, but it could be bad anyway depending on what exactly you plan on doing with the task).

You also can't be sure that the list is not empty when you try to get a task from it: it was empty when it last checked in the while-loop, but between then and the time that it attempts to take a task, another thread might have already taken the last task. This is true even if you remove the call to Thread.sleep.

What you need to is make sure that the availableTasks list is only modified by one thread at a time. This can be done in various ways, for example by using a Semaphore, or by using synchronized methods in the shared data object.

answered Dec 10, 2016 at 20:54

Lidae

3282 silver badges7 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

Juan M Over a year ago

thanks @Lidae, i used a synchronized method and the problem was solved

uylmz Over a year ago

I think he also needs a method that can select an index and remove the element at that index in one atomic operation so different threads don't get the same task.

uylmz · Accepted Answer · 2016-12-10 20:43:35Z

1

it's always the same thread running 10 times

My guess is because your list is too small, so first thread runs and finishes the job before second thread have a chance to start working. Make task list longer, like 1000+ tasks, maybe even more.

it either runs 2 times

this is probably because your task list is not thread safe, make it thread safe using Collections.SynchronizedList

    for (int i = 1; i <= 10; i++) {

        taskList.add(new Task("some details");
    }

    taskList = Collections.synchronizedList(taskList);

    TaskManager manager = new TaskManager();

answered Dec 10, 2016 at 20:43

uylmz

1,5802 gold badges26 silver badges50 bronze badges

2 Comments

Patrick Parker Over a year ago

Collections.synchronizedList is not good enough. There is nothing preventing two threads from working the same task.

uylmz Over a year ago

I noticed his thread sleeps for 1 second before each task, so my first statement is probably not correct too.

lexicore · Accepted Answer · 2016-12-10 21:00:50Z

Can't reproduce this.

I've corrected (quite a few) compilation problems with your code and ran it, getting:

Thread-1 printing some details for task 5
Thread-0 printing some details for task 8
Thread-0 printing some details for task 2
Thread-1 printing some details for task 7
Thread-1 printing some details for task 1
Thread-0 printing some details for task 3
Thread-0 printing some details for task 6
Thread-1 printing some details for task 9
Thread-0 printing some details for task 4
Thread-1 printing some details for task 0

So both threads run and process tasks.

One thing which is important is that the access to the list of tasks should be synchronized. And not just Collections.synchronizedList, you have at least four places where you access your list of tasks. This is why the execution of the program almost always ends with:

java.lang.IllegalArgumentException: n must be positive
    at java.util.Random.nextInt(Random.java:250)
    at TaskManager.takeTask(TaskManager.java:25)
    at TaskManager.run(TaskManager.java:18)
    at java.lang.Thread.run(Thread.java:662)

Your TaskManager.run method first check for isEmpty and then gets a random task from the list. Another thread may remove the last task of the list between these two operations. Resulting in random.nextInt(0) despite you've previously checked that the list is not empty.

Better would be something like:

private Task nextTask() {
    synchronize(availableTask) {
        if (availableTask.isEmpty()) {
            return null;
        } else {
            return availableTasks.get(random.nextInt(availableTasks.size()));
        }
    }
}

dvsakgec · Accepted Answer · 2016-12-10 21:41:17Z

0

Adding to what @Lidae answered. It is standard multiple-producer to multiple-consumer problem. There are couple of articles on the same..

answered Dec 10, 2016 at 21:41

dvsakgec

3,8904 gold badges31 silver badges36 bronze badges

Collectives™ on Stack Overflow

Java Multithreading - Remove items from list

4 Answers 4

2 Comments

2 Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

2 Comments

2 Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related