This discussion is archived
6 Replies Latest reply: Mar 12, 2013 7:11 PM by EJP RSS

Testing Concurrency

916903 Newbie
Currently Being Moderated
I have multiple producers producing prices for various entities, and multiple consumers consuming prices. I have wrriten a class which holds the latest price for each entity.

How can I test that the class I have wrriten is thread safe?
interface IPriceHolder {
     public void putPrice(String e, BigDecimal p);

     public BigDecimal getPrice(String e);

     public boolean hasPriceChanged(String e);

     public BigDecimal waitForNextPrice(String e) throws InterruptedException;
}

class PriceHolder2 implements IPriceHolder {
     private static final long WAIT_TIMEOUT_MS = 10000L;
     private final Map<String, ValueLatch> valuesByEntityMap;
     private final Lock lock;

     public PriceHolder2() {
          valuesByEntityMap = new ConcurrentHashMap<>();
          lock = new ReentrantLock();
     }

     @Override
     public void putPrice(String e, BigDecimal p) {
          try {
               lock.lock();
               if (valuesByEntityMap.containsKey(e)) {
                    valuesByEntityMap.get(e).put(p);
               } else {
                    valuesByEntityMap.put(e, new ValueLatch(e, p));
               }
          } finally {
               lock.unlock();
          }

     }

     @Override
     public BigDecimal getPrice(String e) {
          return valuesByEntityMap.get(e).get();
     }

     @Override
     public boolean hasPriceChanged(String e) {
          return valuesByEntityMap.get(e).hasValueChanged(e);
     }

     @Override
     public BigDecimal waitForNextPrice(String e) throws InterruptedException {
          return valuesByEntityMap.get(e).waitForNextValue();
     }

     class ValueLatch {
          private final String entity;
          private BigDecimal value;
          private CountDownLatch latch;
          private boolean read;
          private final Lock lock;

          public ValueLatch(String entity, BigDecimal value) {
               this.value = value;
               this.entity = entity;
               this.latch = new CountDownLatch(1);
               this.lock = new ReentrantLock();
               this.read = false;
          }

          public void put(BigDecimal value) {
               try {
                    lock.lock();
                    this.latch.countDown();
                    this.value = value;
                    this.read = false;
               } finally {
                    lock.unlock();
               }
          }

          public boolean hasValueChanged(String e) {
               try {
                    return !read;
               } finally {
                    lock.unlock();
               }
          }

          public BigDecimal get() {
               try {
                    lock.lock();
                    this.read = true;
                    return value;
               } finally {
                    lock.unlock();
               }
          }

          public BigDecimal waitForNextValue() throws InterruptedException {
               System.out.println("waiting for next price " + entity + ": "
                         + Thread.currentThread().getName());
               boolean b = latch.await(WAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS);
               if (!b) {
                    System.out.println("Waiting for " + entity + " has timed out");
               }
               return get();
          }
     }
}
  • 1. Re: Testing Concurrency
    Kayaman Guru
    Currently Being Moderated
    913900 wrote:
    How can I test that the class I have wrriten is thread safe?
    Run multiple threads that use a single instance of the class and check that the results are consistent with what you're expecting?
    Of course that's not an exhaustive approach, but it'll at least show if you've completely bollocksed it.
  • 2. Re: Testing Concurrency
    916903 Newbie
    Currently Being Moderated
    I can write multi-threaded code to use the price map, but what im not sure of is how to assert if the value I get from the map is the right value at that exact time?

    I read that to test concurrent queue implementations, a good way is to test the sum of the values you add the queue, is the sum that you take from the queue. I have been trying to adapt this idea to the map, but have so far struggled to come up with a good method.
         @Test
         public void testGetPut() throws InterruptedException {
              int nProducers = 5;
              int nConsumers = 5;
              final ExecutorService executor = Executors
                        .newFixedThreadPool(nProducers + nConsumers);
              CountDownLatch latch = new CountDownLatch(1);
              int nTrials = 10000;
              for (int i = 0; i < nProducers; i++) {
                   executor.execute(new Producer(nTrials, ph, latch));
              }
              for (int i = 0; i < nConsumers; i++) {
                   executor.execute(new Consumer(nTrials, ph, latch));
              }
              latch.countDown();
         }
    
         class Producer implements Runnable {
              private final IPriceHolder ph;
              private final CountDownLatch latch;
              private final int nTrials;
    
              public Producer(int nTrials, IPriceHolder ph, CountDownLatch latch) {
                   this.ph = ph;
                   this.latch = latch;
                   this.nTrials = nTrials;
              }
    
              @Override
              public void run() {
                   try {
                        this.latch.await();
                        for (int i = 0; i < nTrials; i++) {
                             ph.putPrice("a", new BigDecimal(i));
                        }
                   } catch (InterruptedException e) {
                        e.printStackTrace();
                   }
              }
         }
    
         class Consumer implements Runnable {
              private final IPriceHolder ph;
              private final CountDownLatch latch;
              private final int nTrials;
    
              public Consumer(int nTrials, IPriceHolder ph, CountDownLatch latch) {
                   this.ph = ph;
                   this.latch = latch;
                   this.nTrials = nTrials;
              }
    
              @Override
              public void run() {
                   try {
                        this.latch.await();
                        for (int i = 0; i < nTrials; i++) {
                             ph.getPrice("a");
                             //what do I assert agisnt here?
                        }
                   } catch (InterruptedException e) {
                        e.printStackTrace();
                   }
              }
         }
  • 3. Re: Testing Concurrency
    jtahlborn Expert
    Currently Being Moderated
    FYI, the "lock()" call should be outside the finally block which contains the "unlock()" call. also "hasValueChanged()" seems to be missing the lock call. also, don't use "containsKey()" followed by "get()", just "get()" and test for null.

    as a general note, testing concurrency (i.e. creating meaningful tests) is severely difficult. good luck!
  • 4. Re: Testing Concurrency
    916903 Newbie
    Currently Being Moderated
    Thanks, ill add the missing lock statment. Calling just get instead of containsKey should help speed things up slightly as well.

    Just out interest why should you call lock outside the try statement?

    Thanks

    Chris
  • 5. Re: Testing Concurrency
    jtahlborn Expert
    Currently Being Moderated
    913900 wrote:
    Just out interest why should you call lock outside the try statement?
    because if the "lock()" call fails, then the lock didn't succeed, in which case you shouldn't be calling "unlock()" and attempting to release the lock you never got. You'll also notice that this is the pattern shown in the javadoc for Lock:

    http://docs.oracle.com/javase/6/docs/api/java/util/concurrent/locks/Lock.html
  • 6. Re: Testing Concurrency
    EJP Guru
    Currently Being Moderated
              public boolean hasValueChanged(String e) {
                   try {
                        return !read;
                   } finally {
                        lock.unlock();
                   }
              }
    Something wrong there. You're unlocking a lock you don't hold.

Legend

  • Correct Answers - 10 points
  • Helpful Answers - 5 points