Is this Java thread locking concept bad?

Question

I will try to briefly explain a thread locking concept which I came up with using an example. Consider the following example program.

public class Main {
    public static void main(String[] args) {
        Data data = new Data();

        while (true) {
            doStuff();
            doStuff();

            for (int i = 0; i < 256; i++) {
                System.out.println("Data " + i + ": " + data.get(i));
            }

            doStuff();
            doStuff();

            for (int i = 0; i < 256; i++) {
                data.set(i, (byte) (data.get(i) + 1));
            }

            doStuff();
            doStuff();
        }
    }

    public static void doStuff() {
        try {
            Thread.sleep(10);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
    }
}

public class Data {
    private final byte[] data = new byte[256];

    public byte get(int i) {
        return data[i];
    }

    public void set(int i, byte data) {
        this.data[i] = data;
    }
}

It is important that only the main thread modifies data. Now I want to make the loop which prints data asynchronous.

    public static void main(String[] args) {
        ExecutorService executorService = Executors.newSingleThreadExecutor();
        Data data = new Data();

        while (true) {
            doStuff();
            doStuff();

            executorService.submit(new Runnable() {
                @Override
                public void run() {
                    for (int i = 0; i < 256; i++) {
                        System.out.println("Data " + i + ": " + data.get(i));
                    }
                }
            });

            doStuff();
            doStuff();

            for (int i = 0; i < 256; i++) {
                data.set(i, (byte) (data.get(i) + 1));
            }

            doStuff();
            doStuff();
        }
    }

After submitting the task to the executorService the main thread can now move on working as desired. The problem is, that the main thread can potentially reach the point where it modifies data before it was printed but the state of data should be printed when it was submitted.

I know in this case I can create a copy of data before submitting which is printed but that's really not what I want to do. Keep in mind that this is just an example and copying could be an expensive operation in the real code.

This is the solution I came up with for this problem.

    public static void main(String[] args) {
        ExecutorService executorService = Executors.newSingleThreadExecutor();
        Data data = new Data();
        Lock lock = new Lock(); // <---------------

        while (true) {
            doStuff();
            doStuff();

            lock.lock(); // <---------------
            executorService.submit(new Runnable() {
                @Override
                public void run() {
                    for (int i = 0; i < 256; i++) {
                        System.out.println("Data " + i + ": " + data.get(i));
                    }

                    lock.unlock(); // <---------------
                }
            });

            doStuff();
            doStuff();

            lock.waitUntilUnlock(); // <---------------
            for (int i = 0; i < 256; i++) {
                data.set(i, (byte) (data.get(i) + 1));
            }

            doStuff();
            doStuff();
        }
    }

public class Lock {
    private final AtomicInteger lockCount = new AtomicInteger();

    public void lock() {
        lockCount.incrementAndGet();
    }

    public synchronized void unlock() {
        lockCount.decrementAndGet();
        notifyAll();
    }

    public synchronized void waitUntilUnlock() {
        while (lockCount.get() > 0) {
            try {
                wait();
            } catch (InterruptedException e) {

            }
        }
    }
}

Now the main thread can move on working on other stuff after submitting data. At least it can until it reaches the point where it modifies data.

The question: Is this a good or a bad design? Or is there a better (already existing) implementation for this problem?

Note that ReentrantLock wont work in this case. I have to lock before submitting on the main thread and release the lock on the executor thread.

Answer 1

Java has higher-level synchronization abstractions. In general, you should really avoid wait() and notifyAll(), which are too low-level and complex to use correctly and read.

In this case, you could just use a shared blocking queue (a synchronous queue looks appropriate to me) between both threads:

    ExecutorService executorService = Executors.newSingleThreadExecutor();
    Data data = new Data();
    SynchronousQueue queue = new SynchronousQueue();

    while (true) {
        doStuff();
        doStuff();

        executorService.submit(new Runnable() {
            @Override
            public void run() {
                for (int i = 0; i < 256; i++) {
                    System.out.println("Data " + i + ": " + data.get(i));
                }
                queue.put(data);
            }
        });

        doStuff();
        doStuff();

        data = queue.take();
        for (int i = 0; i < 256; i++) {
            data.set(i, (byte) (data.get(i) + 1));
        }

        doStuff();
        doStuff();
    }

Answer 2

You want this Data thing to be built asynchronously, and the main thread wants to be able to proceed to a certain point, then needs to get the completed object. This is what Futures are for, it gives you a reference to a computation that may not have completed yet.

Rewrite the async part as a Callable so that it returns a Data as a result.

Callable<Integer> task = () -> {
    Data data = new Data();
    for (int i = 0; i < 256; i++) {
        System.out.println("Data " + i + ": " + data.get(i));
    }
    return data;
};

ExecutorService executor = Executors.newFixedThreadPool(1);
Future<Data> future = executor.submit(task);
doStuff();
// ... main thread goes about its business

// when you get to a point where you need data, 
// you can block here until the computation is done
Data data = future.get(); 

This way the Future handles making sure the Data object is made visible across threads.

Answer 3

I found an alternative approach to the problem which also answers my comment from the answer of @JB Nizet. ExecutorService#submit(Runnable) returns a Future<?> which can be used to wait until the task is ready. If multiple submissions should be possible one can just create a Queue<Future<?>> queue , always offer the Future<?> returned by ExecutorService#submit(Runnable) to the queue and at the point where the main thread should wait just #poll().get() the entire queue.

Edit: I have also found a related answer here: https://stackoverflow.com/a/20496115/3882565

Answer 4

This seems to be covered well enough by basic synchronization: doStuff expects to have sole access to the data, presumably so that it can modify the data as a whole safely. Meanwhile, doPrint expects to operation on stable data. In these two cases, the data is the unit of state to which access is controlled, and the appropriate locking technique is to synchronize on the data instance which is being accessed.

public void runningInThread1() {
    Data someData = getData(); // Obtain the data which this thread is using
    doStuff(someData); // Update the data
    // Looping and such omitted.
}

public void runningInThread2() {
    Data someData = getData();
    doPrint(someData); // Display the data
}

public void doStuff(Data data) {
    synchronized ( data ) {
        // Do some stuff to the data
    }
}

public void doPrint(Data data) {
    synchronized ( data ) {
        // Display the data
    }
}

Alternatively, if doStuff and doPrint are implemented as instance methods of Data, then the synchronization could be made to happen by adding the synchronized keyword to the instance methods.