Parallel task with synchronized arraylist

Question

My homework was to create project using parallelization that all should be proper. However, I made my project but my profesor mentioned something is wrong in my code "please look at array list, something is not ok, maybe synchronization?".

I would like ask you community to help me and point what could be wrong. I think it might be problem with not covering by synchronize brackets my array list, am I right?

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.logging.Level;
import java.util.logging.Logger;

    /**
    * My project finds all dividors for specific number
    *It must use threads, so I made them. First I start them (first loop) 
    *then join them (second loop). My project must have that loops.
    *Problem might be with not synchronizing methods array list...
    */

    public class Main {

        private final static int NUMBER = 100;
        private final static List<Integer> dividors = new ArrayList<Integer>();

        public static void main(String[] args) {
            new Main().doStuff();
        }
        private int sqr;
        private int sqrp1;

        private void doStuff() {

            sqr = (int) Math.sqrt(NUMBER);
            sqrp1 = sqr + 1;

            Thread[] t = new Thread[sqrp1];

        //starting tasks
            for (int i = 1; i < sqrp1; i++) {
                final int it = i;

                if (NUMBER % i == 0) {
                    final int e = i;

                    t[i] = new Thread(new Runnable() {
                        @Override
                        public void run() {
                            System.out.println("sta"+e);
                            if (!checkContains(e)) {
                                addElement(e);
                            }

                            final int dividednumber = NUMBER / e;

                            if (!checkContains(dividednumber)) {
                                addElement(dividednumber);
                            }
                        }
                    });
                    t[i].start();
                }
            }

        //calling join for tasks
            for (int i = 1; i < sqrp1; i++) {
                final int it = i;

                if (NUMBER % i == 0) {
                    try {
                        System.out.println("sto"+i);
                        t[i].join();
                    } catch (InterruptedException ex) {
                        Logger.getLogger(Main.class.getName()).log(Level.SEVERE, null, ex);
                    }
                }
            }


            System.out.println("xxx");

            Collections.sort(dividors);
            Integer[] arrayDividors = dividors.toArray(new Integer[0]);

            for (int i = 0; i < arrayDividors.length; i++) {
                System.out.println(arrayDividors[i]);
            }
        }

        private synchronized void addElement(int element) {
            dividors.add(element);
        }

        private synchronized boolean checkContains(int element) {
            return dividors.contains(element);
        }
    }

Am I right changing this part, is it ok now?

t[i] = new Thread(new Runnable() {
    @Override
    public void run() {
        System.out.println("waiting " + e);
        synchronized (this) {
            System.out.println("entering " + e);

            if (!checkContains(e)) {
                addElement(e);
            }

            final int dividednumber = NUMBER / e;

            if (!checkContains(dividednumber)) {
                addElement(dividednumber);
            }
            System.out.println("leaving " + e);
        }
    }
});

BTW Your condition should read i <= sqrp1 otherwise square numbers will miss a factor. — Peter Lawrey
– Peter Lawrey, Commented May 31, 2013 at 8:00

Peter Lawrey · Accepted Answer · 2013-05-31 07:34:49Z

1

You need to turn this into a single atomic operation.

 if (!checkContains(dividednumber)) {
     addElement(dividednumber);
 }

Imagine you have two threads.

 T1:  if (!checkContains(dividednumber)) { // false
 T2:  if (!checkContains(dividednumber)) { // false
 T1:      addElement(dividednumber); // adds number
 T2:      addElement(dividednumber); // adds same number

If you have one addElementWithoutDuplicates, this won't happen.

answered May 31, 2013 at 7:34

Peter Lawrey

535k83 gold badges770 silver badges1.2k bronze badges

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

6 Comments

Santosh Over a year ago

specific to this problem, dividednumber should not be same for any two threads as its obtained from an increasing counter. So adding the same number does not seems likely.

Peter Lawrey Over a year ago

@Santosh A number can be added twice in theory, but the upper bound prevents this.

deadfish Over a year ago

okay, but checkContains is synchronized, right? Does it means I should cover this code by synchronize(this){ ... } ?

Peter Lawrey Over a year ago

@Fishonthegrass correct. Another thread can run in between two synchronized methods. Just because two methods are thread safe, doesn't mean they can be used in any combination and still be thread save.

deadfish Over a year ago

I edited my qustion, is it ok now? @PeterLawrey , should I cover synchronized(this) also my synchronized 2 methods?

|

Collectives™ on Stack Overflow

Parallel task with synchronized arraylist

1 Answer 1

6 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

6 Comments

Your Answer

Sign up or log in

Post as a guest

Related