Should input listeners be synchronized?

Question

My sample code posted below shows two classes. One implements KeyListener and the other implements Runnable and is running in an infinite loop sleeping every 20 ms. When a key is pressed the keyChar, which is in the form of an int, is used as an index setting the index of a boolean array true or false, representing with the key was pressed or not. At the same time the process loop is searching the key array for its true or false values and setting the true ones false then printing out the char. My question is whether or not I need to use synchronization using a lock for accessing the charArray because it is used in two threads: the process thread and the key listener thread.

Sample Code:

import java.awt.Component;
import java.awt.event.KeyEvent;
import java.awt.event.KeyListener;

public class Input implements KeyListener {

public boolean[] charArray;

public Input(Component component) {
    charArray = new boolean[127];
    component.addKeyListener(this);
}

@Override
public void keyPressed(KeyEvent e) {
            (possible synchronization with a lock?)
    int keyChar = e.getKeyChar();
    if (keyChar == 27 || keyChar == 9 || keyChar == 10 || keyChar == 127) //useless keys like del, tab, esc, etc..
        keyChar = 65535;
    if (keyChar < 65535) //65535 represents no true char value
        charArray[keyChar] = true;
}

@Override
public void keyReleased(KeyEvent e) {
}

@Override
public void keyTyped(KeyEvent e) {
}
}




import java.awt.Dimension;
import javax.swing.JFrame;

@SuppressWarnings("serial")
public class Process extends JFrame implements Runnable {

private boolean running;
private Input input;

public Process() {
    running = false;
    input = new Input(this);
    setTitle("Keyboard Test");
    setSize(new Dimension(200, 200));
    toFront();
    setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    setVisible(true);
}

/**
 * @param args
 */
public static void main(String[] args) {
    new Process().startThread();
}

public synchronized void startThread() {
    running = true;
    new Thread(this).start();
}

@Override
public void run() {
    while (running) {
                    (possible synchronization with a lock?)
        for (int i = 0; i < input.charArray.length; i++) {
            if (input.charArray[i] == true) {
                input.charArray[i] = false;
                System.out.println((char) i);
            }
        }
        try {
            Thread.sleep(20);
        } catch (InterruptedException e) {
        }
    }
}
}

Answer 1

When it comes to AWT or Swing the number one rule is do not ever synchonize or otherwise interfere with the dispatch thread. If you're not familiar with this then take a look at Dispatch Thread Issues

In your case I would separate the non GUI thread functionality into a separate class altogether - and use one of the really useful classes in java.util.concurrent to communicate between the two if necessary.

If you get a lock or delay due to threading issues and you're actually in the Dispatch Thread whole GUI will freeze

Answer 2

Your charArray variable is accessed from at least two threads (the one you start in Process and the EDT in your Input class) so you need to synchronize those accesses to ensure visibility (i.e. make sure that changes made by one thread are visible from the other thread).

Note that there are several other issues in your code, for example:

• you should not let this escape during construction (by calling input = new Input(this) or component.addKeyListener(this)) - this can lead to weird behaviour in a multi-threaded environment
• you should try to have a JFrame variable inside your Process class instead of extending JFrame
• I'm not sure how you plan to set running to false, but there is no synchronization around that variable in your run method so you might not see it become false.