Why don't all methods that concurrently modify a shared object need the synchronized modifier?

Question

I would like to understand when to add the synchronized modifier to a method that modifies a shared object and when not.

I wrote a modified version of the bouncing balls game. Each ball (I called it "stone") is a thread. To manage the repaint process, I keep an HashSet of stones and several methods process this set:

• when I add a new stone (if the user presses the Fire button on the GUI);

• when I kill one or more stones (if the user clicks the stones on the panel);

• when one of the killer stones (a special "bad" kind of stones) touches any of the normal "good" stones and kills them;

• and finally, when the paintComponent() method is called.

Well, I thought the all of the methods that process those things must be declared synchronized. But I made some attempts and I found that:

• in some cases the synchronized modifier is needed (if I remove it, I get an exception, OK that's what I expected);

• in some other cases I removed the synchronized modifier and I never got any exception, even running the program more and more, creating and killing tons of killer stones and/or good stones.

I googled a lot but I have read that the synchronized modifier is always needed when a method accesses a shared object and it isn't needed only if the object is immutable. But then I can't understand why I can remove the synchronized modifier from some of those methods without getting exceptions.

Now I attach the StoneSet class, which is where all those methods are defined. This class is a singleton: only one instance of it is created and shared by almost all other objects in the application. I cleaned the class from all the unnecessary code and wrote many comments to help readers understand the class and (I hope) tell me what is happening. I apologize for the long attachment (more than 100 lines).

package rollingstones;

import java.awt.Graphics;
import java.util.HashSet;
import java.util.Iterator;

class StoneSet {

    private final HashSet<Stone> set = new HashSet<>(64);       // this is the set
    private AreaGrafica areaGrafica;                            // this is the JPanel

    void setAreaGrafica(AreaGrafica areaGrafica) {              // invoked at the beginning
        this.areaGrafica = areaGrafica;                         
    }

    /**
     * This method is called by the paintComponent() of the panel.
     * HERE THE SYNCHRONIZED MODIFIER IS NEEDED: IF I REMOVE IT, I GET java.util.ConcurrentModificationException
     */
    synchronized void redrawAll(Graphics g) {
        final Iterator<Stone> iter = set.iterator();
        Stone stone;
        while (iter.hasNext()) {
            stone = iter.next();
            g.setColor(stone.getColor());
            g.fillOval(stone.getX(), stone.getY(), stone.getSize(), stone.getSize());
        }
    }

    /**
     * This method is called when the user clicks the GUI's Fire button (actionPerformed awt event).
     */
    void addGoodStone() {
        Stone stone = new GoodStone();              // GoodStone is a Stone
        addStone(stone);
    }

    /**
     * This method is called when the user clicks the GUI's Killer button (actionPerformed awt event).
     */
    void addKillerStone() {
        Stone stone = new KillerStone();            // KillerStone is a Stone
        addStone(stone);
    }

    /**
     * This method adds a stone into the set, so it modifies the set, but...
     * ...HERE I REMOVED THE SYNCHRONIZED MODIFIER AND I NEVER GOT ANY EXCEPTION.
     */
    private void addStone(Stone stone) {
        stone.start();                              // start the thread (each stone is a thread)
        set.add(stone);                             // put the stone into the set
        System.out.print(set.size() + " ");
    }

    /**
     * This method is called when the user clicks a point on the panel (mouseClicked awt event).
     * This method removes more than one of the stones from the set, but...
     * ...HERE I REMOVED THE SYNCHRONIZED MODIFIER AND I NEVER GOT ANY EXCEPTION.
     */
    void killStone(int xClicked, int yClicked) {
        final Iterator<Stone> iter = set.iterator();
        Stone stone;

        while (iter.hasNext()) {
            stone = iter.next();

            if (SOME CONDITIONS, READING THE STONE STATUS) {
                stone.interrupt();                      // stop the thread
                iter.remove();                          // remove the stone from the set
                System.out.print(set.size() + " ");
            }
        }

        if (set.isEmpty()) {
            areaGrafica.repaint();                      // remove the image of the final stone from the panel
        }
    }


    /**
     * This method is called by the run() method of the killer stones (see later).
     * HERE THE SYNCHRONIZED MODIFIER IS NEEDED: IF I REMOVE IT, I GET java.util.ConcurrentModificationException
     */
    synchronized void killNeighbouringGoodStones(int x, int y, int radius) {
        final Iterator<Stone> iter = set.iterator();
        Stone stone;

        while (iter.hasNext()) {
            stone = iter.next();

            if (SOME OTHER CONDITIONS, USING THE STONE STATUS) {
                stone.interrupt();                      // stone is a thread
                iter.remove();                          // remove the stone from the set
                System.out.print(set.size() + " ");
                }
            }
        }
    }

}

---

/**
 * This is the run() method of the Stone class.
 */
@Override
public void run() {
    try {                                   // while into the try
        while (true) {
            animate();                      // this simple method changes the stone state (*)
            Stone.areaGrafica.repaint();
            Thread.sleep(SLEEP);            // SLEEP is 50 ms
        }
    } catch (InterruptedException ex) {
        System.err.println(ex.getMessage());
    }
}

(*) if the stone is a killer stone, the animate() method is overridden:
@Override
void animate() {
    super.animate();
    set.killNeighbouringGoodStones(getCenterX(), getCenterY(), getSize() / 2);      // here set is the singleton StoneSet
}

---

/**
 * This is the paintComponent() method of the panel.
 */
@Override
protected void paintComponent(Graphics g) {
    super.paintComponent(g);                    
    set.redrawAll(g);
}

I thought the synchronized modifier was mandatory for all methods that access a shared object, but apparently this is not true.

EDIT

I had an idea: maybe...

• ...redrawAll() and killNeighbouringGoodStones() need the synchronized modifier because those methods are called by other threads I created, while...

• ...addStone() and killStone() may not be synchronized because those methods are called by GUI-enabled API listeners.

Could it be true?

Answer 1

1. There is a world of difference between: "I am not getting exceptions", "the code is correct" and "the code, as currently deployed, works correctly".

2. If a mutable object is protected by a synchronized block, EVERY access to that object MUST be protected by a block synchronized on the SAME monitor. Otherwise the code is not thread safe and bad things can happen (and this goes both for reading and for writing).

3. If a code is not thread safe, it might work well enough, especially, if you are writing a game, not a banking system. The really tough errors could appear only under specific circumstances: one in billion executions, on some specific version of JVM, when some additional condition is true etc.

4. The exceptions you are getting are probably not typical threading errors: they come from the fact that HashSet is a fail-fast collection and you are not allowed to remove elements from it using different iterators at the same time. To put it in different words: even if you synchronized the HashSet itself, you would still get this error, even though - from pure thread safety perspective - the code would be correct.

5. As Solomon Slow already commented, your design is not really suited for using threads. So I assume you are doing this for fun / to learn Java threading. It is then especially important to do things correctly :-)

Answer 2

You use sychronized if multiple threads try to change the same object at the same time. As you noticed if remove synchronized from some of you methods you get an exception, because all of them try to modify the HashSet at the same time.

You must ensure that the following things to not happen at the same time (triggered by two different Threads):

1. Iteration over the values in the the HashSet
2. Modification of the Data inside the HashSet

From a codestyle point of view, synchronized is never 'mandatory'. Its up to the developer to figure out where to use it, so that your code works correctly.

You also want to take a look at the 'volatile' keyword to ensure that all threads see the same content.