CODEWITHSUNDEEP
javamultithreadinglist
Peck3277
Peck3277

Reputation: 1423

Java: What type of list to use in my multi-threaded app

I'm new to using threads and just trying to figure things out. My end game is to have a list of URLs, my program will take one URL from the list at a time and perform an action using that URL. There'll be a lot of URLs and this list may possibly be added to while some threads are using the same list.

To start experimenting and learning I'm using a simple ArrayList filled with numbers and am using a threaded pool to get the URLs. Here's my code:

public static void main(String[] args) {

    for (int i = 0; i < 200; i++){
        test.add(i);
    }

    SlothTest runner = new SlothTest();
    Thread alpha = new Thread(runner);
    Thread beta = new Thread(runner);

    ExecutorService tasker = Executors.newFixedThreadPool(10);

    while (!listEmpty()){
        tasker.submit(new SlothTest());
    }

    tasker.shutdown();
    System.out.println("Complete...");
}

@Override
public void run() {
    getLink();
    try {
        Thread.sleep(20);
    } catch (InterruptedException e) {
    }
}

private synchronized String getLink(){
    link = Thread.currentThread().getName() + " printed " + test.indexOf(test.size()-1);
    test.remove(test.size()-1);
    System.out.println(link);
    return link;
}

private synchronized static boolean listEmpty(){
    if (test.size() > 0){
        return false;
    } else {
        return true;
    }
}

I'm running into some concurrency issues while running the program and getting some -1's for my output. I'm not sure why this is happening and I know my above code is rough but I'm really in the learning stage a multi-threaded apps. Can anyone help me first off with fixing my concurrency issue and then if you can give me any pointers about my above code that would also be great

Upvotes: 2

Views: 165

Answers (3)

Tudor
Tudor

Reputation: 62459

You're not globally synchronizing. By using synchronized methods you are locking the current instance, which is different for each task. You should use a global lock instead:

final static Object globalLock = new Object();

private String getLink() {
   synchronized (globalLock) {
      link = Thread.currentThread().getName() + " printed " + test.indexOf(test.size()-1);
      test.remove(test.size()-1);                    
   }
   System.out.println(link);
   return link;
}

private boolean listEmpty(){
    synchronized (globalLock) {
       if (test.size() > 0){
          return false;
       } else {
          return true;
       }
    }
}

Upvotes: 2

bfishman
bfishman

Reputation: 579

Try using a ConcurrentLinkedQueue for your list of URLs. This is a good implementation often used in producer-consumer examples, similar to yours (although you don't have an active 'producer', per-se).

Upvotes: 2

assylias
assylias

Reputation: 328737

One problem is that

while (!listEmpty()){
    tasker.submit(new SlothTest());
}

is not atomic. So listEmpty might return false, but become true by the time you reach the next statement.

Another one is that you synchronize on two different monitors:

private synchronized String getLink(){ //synchronized on this

private synchronized static boolean listEmpty(){//synchronized on this.class

Have you considered using a BlockingQueue instead of a list, which has useful methods for what you are trying to achieve.

Upvotes: 3

Related Questions