3
\$\begingroup\$

In this program, there is a list of Holders that is managed by a singleton class. Each of the Holders keeps a copy of the external resource (represented by a String here). When adding a new Holder via addHolder(), the current value of the external resource is copied. At anytime, the external resource can update its value and all previously added Holders are notified to update their values (simplified with updateHolders(String) here). The aim is to make sure each Holder's copy of the external resource is up-to-date.

Please review the following examples to demonstrate my progressive improvements on achieving concurrency control:

import java.util.Queue;
import java.util.concurrent.*;

public class ThreadTest1 {
    private static final ThreadTest1 INSTANCE = new ThreadTest1();
    private Queue<Holder> holders = new ConcurrentLinkedQueue<>();
    public static ThreadTest1 instance() {
        return INSTANCE;
    }
    public void updateHolders(String resource) {
        System.out.println("Updating holders " + holders);
        holders.forEach(h -> h.setResource(resource));
        System.out.println("Finished updating holders " + holders);
    }
    public void addHolder(int delay) throws InterruptedException {
        Holder holder = new Holder();
        holder.setResource(ExternalResource.getResource());
        System.out.println("Adding holder " + holder);
        TimeUnit.SECONDS.sleep(delay);  // more work to do
        holders.add(holder);
        System.out.println("Finished adding holder " + holder);
    }
    public Queue<Holder> getHolders() {
        return holders;
    }

    public static class Holder {
        private static int holderId = 1;
        private int id;
        private String resource;
        public Holder() {
            this.id = holderId++;
        }
        public void setResource(String resource) {
            this.resource = resource;
        }
        @Override
        public String toString() {
            return "H" + id + "[" + resource + "]";
        }
    }

    public static class ExternalResource {
        private static String resource = "A";
        public static String getResource() {
            return resource;
        }
        public static void setResource(String resource) {
            ExternalResource.resource = resource;
            System.out.println("External resource changed to " + resource);
            ThreadTest1.instance().updateHolders(resource);
        }
    }

    public static Callable<Void> callAddHolderWithDelay(int delay) {
        return () -> {
            ThreadTest1.instance().addHolder(delay);
            return null;
        };
    }

    public static Callable<Void> callSetResource(String resource) {
        return () -> {
            ExternalResource.setResource(resource);
            return null;
        };
    }

    public static void main(String[] args) throws ExecutionException, InterruptedException {
        ExecutorService executor = Executors.newFixedThreadPool(10);
        executor.submit(callAddHolderWithDelay(1));
        TimeUnit.SECONDS.sleep(1);
        executor.submit(callAddHolderWithDelay(5));
        TimeUnit.SECONDS.sleep(1);
        executor.submit(callSetResource("B"));
        TimeUnit.SECONDS.sleep(1);
        executor.submit(callAddHolderWithDelay(1));
        TimeUnit.SECONDS.sleep(5);
        System.out.println(ThreadTest1.instance().getHolders());
        executor.shutdown();
    }
}

Output:

Adding holder H1[A]
Adding holder H2[A]
Finished adding holder H1[A]
External resource changed to B
Updating holders [H1[A]]
Finished updating holders [H1[B]]
Adding holder H3[B]
Finished adding holder H3[B]
Finished adding holder H2[A]
[H1[B], H3[B], H2[A]]

First test is the most basic one without any concurrency control, as you can see an unfortunate event happened and H2 ended up with holding A instead of B.

An improvement was made to make sure the above won't happen as follow (only highlighting the changes):

public class ThreadTest2 {
    private static final ThreadTest2 INSTANCE = new ThreadTest2();
    private Queue<Holder> holders = new ConcurrentLinkedQueue<>();
    private ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
    public static ThreadTest2 instance() {
        return INSTANCE;
    }
    public void updateHolders(String resource) {
        lock.writeLock().lock();
        System.out.println("Updating holders " + holders);
        holders.forEach(h -> h.setResource(resource));
        System.out.println("Finished updating holders " + holders);
        lock.writeLock().unlock();
    }
    public void addHolder(int delay) throws InterruptedException {
        lock.readLock().lock();
        Holder holder = new Holder();
        holder.setResource(ExternalResource.getResource());
        System.out.println("Adding holder " + holder);
        TimeUnit.SECONDS.sleep(delay);  // more work to do
        holders.add(holder);
        System.out.println("Finished adding holder " + holder);
        lock.readLock().unlock();
    }
    ...

Output:

Adding holder H1[A]
Adding holder H2[A]
Finished adding holder H1[A]
External resource changed to B
Finished adding holder H2[A]
Updating holders [H1[A], H2[A]]
Finished updating holders [H1[B], H2[B]]
Adding holder H3[B]
Finished adding holder H3[B]
[H1[B], H2[B], H3[B]]

Test 2 is a typical use of a read write lock and the result is correct. However, it is not very efficient as you can see Adding holder H3[B] was pushed until after Finished updating holders [H1[B], H2[B]], but really it should not be blocked because External resource changed to B had happened already and hence Adding holder H3[B] could pick up the new value with no reason to wait for Finished updating holders [H1[B], H2[B]].

So what I can optimise here is a non-blocking addHolder and a blocking updateHolders, where it blocks only when there is one or more threads working on addHolder already (e.g. H2) but fine to ignore subsequent calls to addHolder (e.g. H3) as they are good on their own to get the latest value. I used a list of locks to implement that:

public class ThreadTest3 {
    private static final ThreadTest3 INSTANCE = new ThreadTest3();
    private Queue<Holder> holders = new ConcurrentLinkedQueue<>();
    private Queue<Lock> locks = new ConcurrentLinkedQueue<>();
    public static ThreadTest3 instance() {
        return INSTANCE;
    }
    public void updateHolders(String resource) {
        locks.forEach(l -> l.lock());
        System.out.println("Updating holders " + holders);
        holders.forEach(h -> h.setResource(resource));
        System.out.println("Finished updating holders " + holders);
    }
    public void addHolder(int delay) throws InterruptedException {
        Lock lock = new ReentrantLock();
        lock.lock();
        locks.add(lock);
        Holder holder = new Holder();
        holder.setResource(ExternalResource.getResource());
        System.out.println("Adding holder " + holder);
        TimeUnit.SECONDS.sleep(delay);  // more work to do
        holders.add(holder);
        System.out.println("Finished adding holder " + holder);
        lock.unlock();
        locks.remove(lock);
    }

Output:

Adding holder H1[A]
Adding holder H2[A]
Finished adding holder H1[A]
External resource changed to B
Adding holder H3[B]
Finished adding holder H3[B]
Finished adding holder H2[A]
Updating holders [H1[A], H3[B], H2[A]]
Finished updating holders [H1[B], H3[B], H2[B]]
[H1[B], H3[B], H2[B]]

Test 3 result is correct and is more efficient than before as you can see Adding holder H3[B] now happens before Finished updating holders [H1[B], H3[B], H2[B]], hence addHolder is non-blocking. I can settle here, but further optimisation seems possible as H1 wasn't updated until Finished adding holder H2[A] where updateHolders was waiting for it to finish, THEN perform the update for all. It can definitely update H1 first as addHolder for H1 had finished and only wait for H2, so H1 can get the update earlier.

Please point out if my use of the list of locks is something an overkill and can be simplified to achieve the same, also whether my further optimisation can be achieved by what means? Thank you.

\$\endgroup\$
  • 1
    \$\begingroup\$ Could you add a specification for the code itself? It's not entirely clear to me what is the target API here (e.g. what are Holder and ExternalResource, and how do they relate to one another?), and some qualification would make it easier to understand your intention with the locking. \$\endgroup\$ – VisualMelon Sep 20 '19 at 9:35
  • \$\begingroup\$ @VisualMelon Added a paragraph to describe more. \$\endgroup\$ – user1589188 Sep 20 '19 at 17:38
  • \$\begingroup\$ Can you give an example input and and the expected behavior? \$\endgroup\$ – Sam Orozco Oct 8 '19 at 3:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.