Use of the Synchronized Keyword in Java

Question

I have read some articles (including Oracle's) on synchronized methods, and I'm not sure if I understand it correctly.

I have the following code:

public class Player extends javax.swing.JLabel implements Runnable{
    private static int off2[] = {-1, 0, 1, 0}, off1[] = {0, 1, 0, -1};
    private static final Map dir = new java.util.HashMap<Integer, Integer>();
    static{
        dir.put(KeyEvent.VK_UP, 0);
        dir.put(KeyEvent.VK_RIGHT, 1);
        dir.put(KeyEvent.VK_DOWN, 2);
        dir.put(KeyEvent.VK_LEFT, 3);
    }
    private boolean moving[] = new boolean[4];

    @Override
    public void run(){
        while(true){
            for(Object i : dir.values()){
                if(isPressed((Integer)i)) setLocation(getX() + off1[(Integer)i], getY() + off2[(Integer)i]);
            }
            try{
                Thread.sleep(10);
            }catch(java.lang.InterruptedException e){
                System.err.println("Interrupted Exception: " + e.getMessage());
            }
        }
    }
    public void start(){
        (new Thread(this)).start();
    }

    private synchronized boolean isPressed(Integer i){
        if(moving[i]) return true;
        else return false;
    }

    public synchronized void setPressed(KeyEvent evt) {
        if(dir.containsKey(evt.getKeyCode()))moving[(Integer)dir.get(evt.getKeyCode())] = true;
    }
    public synchronized void setReleased(KeyEvent evt){
        if(dir.containsKey(evt.getKeyCode()))moving[(Integer)dir.get(evt.getKeyCode())] = false;
    }
}

Right now all it does is movement. My understanding is that the setPressed and setReleased are called from the main thread, when my main form's key listener registers the keyPressed and Released events. Is this code reasonable? Is it a proper use of the synchronized keyword? (The code works without it, but I suspect it's better to have it?)

suspects is a bad reason to do something, and incorrect in this situations as well. — user177800
– user177800, Commented May 25, 2012 at 18:48
Can you tell me which two threads will be calling any of these methods concurrently? — user177800
– user177800, Commented May 25, 2012 at 18:55
if(moving[i]) return true; else return false; should just be return moving[i] — user177800
– user177800, Commented May 25, 2012 at 18:56
True enough, that method was hastily written. I have a main frame that handles keyboard input and then calls the "Player" classes methods. So the movement thread within "Player" and the main thread could access the moving[] array at the same time. I can edit in the code if so desired, but I figured it wasn't necessary. — Daniel
– Daniel, Commented May 25, 2012 at 18:59

Marco Massenzio · Accepted Answer · 2012-05-25 19:08:27Z

1

Not sure about the Swing side of things, but generally speaking you need the synchronized to protect those shared data (the moving[] in your case) that may be accessed by multiple threads.

In this case, you need to sync, as moving[] may be accessed on writing by either of the setXxx methods (main thread) or on read by the thread you started.

since Java 5, the facilities in the java.concurrent package are much better, and I would suggest you consider either using a Lock to protect moving[] or an AtomicBoolean.

A couple of "questions of style":

don't use 'raw types' -- prefer Map<Integer, Integer> to Map (that also saves you from the unchecked -ugly- cast in the setXxxx() methods)
avoid one-line if's -- make your code illegible :)

If you decide to use Lock (not a big advantage v. synchronized in this case) your isPressed() should look something like:

// ...
private Lock movingLock = new ReentrantLock();

private  boolean isPressed(Integer i){
  try {
    movingLock.acquire();
    return moving[i];
  } finally
    movingLock.release();
  }        
}

you would have to 'wrap' the assignments to moving[] in the setXxx methods with calls to Lock.acquire() and release()

answered May 25, 2012 at 19:08

Marco Massenzio

3,0101 gold badge28 silver badges41 bronze badges

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

1 Comment

Daniel Over a year ago

All right. Thank you very much. This answer cleared some things up for me. First of all, clearly I'm not crazy, because it seems you understood what I meant about multiple threads accessing it, haha. Second, thanks for the tip on the raw types. That cast was bothering me, but I didn't know how to fix it. Third, thanks for the tip on locks, but can you help me understand the difference between what the Lock does and what the synchronized keyword does?

Stefan Haustein · Accepted Answer · 2012-05-25 19:26:42Z

1

The use of synchronized is correct, but I don't think it is necessary because in the current implementation, parallel access to the array will not cause inconsistencies.

However, I think there is a different threading issue in your code. You are not supposed interact with Swing (=call setLocation()) from a separate thread, see http://docs.oracle.com/javase/1.4.2/docs/api/javax/swing/SwingUtilities.html#invokeLater(java.lang.Runnable). So you need to wrap the update code into a Runnable:

private boolean moving[] = new boolean[4];
Runnable dirUpdate = new Runnable() {
  for(Object i : dir.values()){
    if(isPressed((Integer)i)) setLocation(getX() + off1[(Integer)i], getY() + off2[(Integer)i]);
  }
};

The invocation from the thread would then look like this:

SwingUtils.invokeLater(dirUpdate);

Note that in this case you won't need synchronized any longer because dirUpdate will be called from the event handling thread.

You may want to check for null in isPressed() to avoid exceptions on unmapped keys.

A simpler representation for the movement may be to have dx and dy variables which would be set to -1, 1 or 0 by the key events.

edited May 25, 2012 at 19:26

answered May 25, 2012 at 19:00

Stefan Haustein

18.9k3 gold badges40 silver badges54 bronze badges

5 Comments

Daniel Over a year ago

Can you help me understand why I'm not supposed to interact with my class from two threads? I looked at that link, but it didn't really help me...

Stefan Haustein Over a year ago

Swing is not thread safe -- basically it does not use synchronized anywhere to keep the design simple and deadlock-free. Thus, when you interact with swing from a separate thread, you need to use invokeLater.

Daniel Over a year ago

Okay thank you. I think I understand now: Through invokeLater() the Runnable is put onto a queue of things to be done? And then they're executed one at a time in a separate thread? Is this correct?

Stefan Haustein Over a year ago

Yes. I have updated the answer with some code snippets to illustrate how it is used.

Daniel Over a year ago

Excellent; thank you. Very helpful! Other than that and the other things you mentioned, the code looks okay, though? And also, why is it that having dirUpdate() called from the event handling thread would eliminate the need for synchronized? Does that thread provide some sort of safety for this?

Collectives™ on Stack Overflow

Use of the Synchronized Keyword in Java

2 Answers 2

1 Comment

5 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

1 Comment

5 Comments

Your Answer

Sign up or log in

Post as a guest

Related