Why is synchronized not working?

Question

I am trying to write a method that asks a device for input and then accepts a response, all as an atomic operation.

Here is my code (the query method is really what should be focused on):

public class DeviceConnection implements Runnable{
    //For query
    static int test = 0;

    //For writeline
    static PrintWriter out = null; //(initialized in constructor)

    //Only important for readline
    static String[] systemMessage=new String[10];
    static int messageIn=0;
    static int messageOut=0;
    static boolean connected = false;
    static boolean endConnect = true;

    static PrintStream logWriter; //(initialized in constructor)
    static String serverName="";//(initialized in constructor)
    static int socketNum;//(initialized in constructor)

    /** REALLY ONLY NEED TO LOOK AT THIS METHOD
         * Atomic operation to ask for data from device
         * and get a response.
         * @param line -    query to be passed to device
         * @return response from device
         */
    public synchronized String query(String line){
        int temp = test;
        System.err.print("foo" + test);
        System.err.print(this);
        String response;
        writeLine(line);
        response = readLine();
        System.err.println("bar" + test + " should be " + temp);
        test = temp+1;
        return response;
    }


/**
     * Writes a query to the device. 
     *<p>
     * Does <b>not</b> get or handle any response
     * @param line -    query to be passed to device
     */
    public synchronized void writeLine(String line) {
        out.println(line + "\n");
    }

/**
     * Reads a response from device.
     *<p>
     * Should never be used outside of <code>query</code>
     * as this could create a race condition if another
     * thread is writing to the device at the same time.
     * @return
     */
    private synchronized String readLine() {

        String response;
        long start, stop;

        if (messageIn != messageOut) { // new message exists
            response = systemMessage[messageOut];
            messageOut = (messageOut + 1) % 10;
        } else {
            start = System.currentTimeMillis();
            try {
                if (connected) { // if connected wait for heatbeats
                    //wait(15000);
                    wait();
                    start = System.currentTimeMillis();
                } else { // if not connected ignore heartbeats
                    wait();
                    start = System.currentTimeMillis();
                }
            } catch (InterruptedException e) { return "Interrupted"; }
            stop = System.currentTimeMillis();

            if (stop - start < 12000) { // heart beats running at 5 s
                if (messageIn != messageOut) { // new message exists
                    response = systemMessage[messageOut];
                    messageOut = (messageOut + 1) % 10;
                } else {
                    return null;
                }
            } else { // heart beats lost
                response = "Heart beats lost";
                logWriter.println(response);
                if (connected) { // connection lost on client side
                    logWriter.println("Connection to " + serverName + 
                                      " lost on client side");
                    sleep(60000);
                    connect(serverName,socketNum);
                }
            }
        }
        return response;
    }
}

Usually the query method runs fine, but sometimes I get output like so:

foo59lib.DeviceConnection@7bd0bf6d(other System.out stuff printed here)
foo59lib.DeviceConnection@7bd0bf6dbar59 should be 59
bar60 should be 59

How is this possible? Aren't the methods locked/synchronized on the object? The object is clearly the same as the prints show, but somehow two query methods are being executed simultaneously.

Answer 1

Your readLine method, which is called from query, calls wait, which releases the lock, which would enable another thread to call query concurrently.

You should always call wait inside a loop using a condition variable, your pattern of using an if to decide whether to wait is flawed. Once your thread reacquires the lock it needs to check what the current state is.

That wait releases the lock is explained in the documentation for Object#wait:

The current thread must own this object's monitor. The thread releases ownership of this monitor and waits until another thread notifies threads waiting on this object's monitor to wake up either through a call to the notify method or the notifyAll method. The thread then waits until it can re-obtain ownership of the monitor and resumes execution.

As in the one argument version, interrupts and spurious wakeups are possible, and this method should always be used in a loop:

 synchronized (obj) {
     while (<condition does not hold>)
         obj.wait();
     ... // Perform action appropriate to condition
 }