2

When the following piece of code is getting executed I am getting exceptions in a random manner.

byte[][] loremIpsumContentArray = new byte[64][];

for (int i = 0; i < loremIpsumContentArray.length; i++)
{
    random.nextBytes(loremIpsumContentArray[i] = new byte[CONTENT_SIZE]);
}

AtomicBoolean aBoolean = new AtomicBoolean(true);
List<Long> resultList = IntStream.range(0, 64* 2)
                                 .parallel()
                                 .mapToObj(i -> getResult(i,
                                                          aBoolean,
                                                          repositoryPath,
                                                          loremIpsumContentArray ))
                                 .collect(Collectors.toList());

getResult function:

try
{
    Repository repository = repositoryPath.getRepository();
    String path = RepositoryFiles.relativizePath(repositoryPath);

    //return aBoolean.compareAndSet(aBoolean.get(), !aBoolean.get()) ?
    return aBoolean.getAndSet(!aBoolean.get()) ?
                              new Store(new ByteArrayInputStream(loremIpsumContentArray[i / 2]), repository, path, lock).call() :
                              new Fetch(repository, path, lock).call();
}

As can be seen from above the code is making use of parallel streams and then calling getResult Function. Also there is an atomic variable involved. When the atomicVariable is true the store function is called and when it is false the fetch function is called.

My understanding is that inside getResult function we are checking and updating atomic variable aBoolean and this check and update operation is atomic but new Store(...).call(); and new Fetch(...).call(); is not and since parallel streams involves multiple threads so there is a race-condition occurring at

return aBoolean.getAndSet(!aBoolean.get()) ?
                          new Store(new ByteArrayInputStream(loremIpsumContentArray[i / 2]), repository, path).call() :
                          new Fetch(repository, path).call();

In order to corroborate my theory of Race condition I added the lock as shown below to both new Store(...).call() and new Fetch(...).call() as shown below and then everything worked fine:

Lock lock = new ReentrantLock();
AtomicBoolean aBoolean = new AtomicBoolean(true);
List<Long> resultList = IntStream.range(0, 64* 2)
                                 .parallel()
                                 .mapToObj(i -> getResult(i,
                                                          aBoolean,
                                                          repositoryPath,
                                                          loremIpsumContentArray,
                                                          lock))
                                 .collect(Collectors.toList());

And getResult function:

return aBoolean.getAndSet(!aBoolean.get()) ?
                          new Store(new ByteArrayInputStream(loremIpsumContentArray[i / 2]), repository, path, lock).call() :
                          new Fetch(repository, path, lock).call();

I have the following questions:

  • Is my understanding correct regarding the race condition occurring as mentioned above and have I used the lock in the way they should be?

  • What are the other ways to avoid the race condition?

Please, let me know your thoughts.

carlspring
  • 31,231
  • 29
  • 115
  • 197
Yug Singh
  • 3,112
  • 5
  • 27
  • 52
  • What exceptions are you getting? What are you doing with your lock? Yes in your current code, aBoolean.get() could be called by two different threads before either one has a chance to call `aBoolean.getAndSet`. Why did you use a lock to check this problem, instead of just removing the 'parallel()' call? – matt Apr 25 '19 at 12:02
  • @matt I am getting `NPE`. Also I am passing the lock in `call()` as shown above and then I am using it as an alternative of synchronized block because `Store` and `Fetch` are two different classes implementing `Callable` and thus overriding `call` method. So I am creating a `lock` object as shown above before calling `new Store() or new Fetch()` and then passing it's reference so that I can use the same lock to synchronize the call methods of both Store and Fetch class. – Yug Singh Apr 25 '19 at 14:13
  • @matt I can't remove the `parallel` as I donot own the code base. – Yug Singh Apr 25 '19 at 14:14
  • Okay, what line gives you the NPE, it doesn't seem like the boolean race condition should cause an NPE. – matt Apr 25 '19 at 15:51
  • If you're interested in finding out more about the issue that this question relates to, and would like to help further, you can have a look at https://github.com/strongbox/strongbox/issues/1248. – carlspring Apr 27 '19 at 14:09

1 Answers1

0

Your aBoolean.getAndSet(!aBoolean.get()) is not atomic. Some thread can hop between !aBoolean.get() and the surrounding aBoolean.getAndSet which might lead to the race condition.

You should synchronize the block:

boolean whatToExec;
synchronized (aBoolean) {
    whatToExec = aBoolean.get();
    aBoolean.set(!whatToExec);
}
return whatToExec ? ...

What the lock is good for in Fetch or Store is unknown. If the race condition happens there, I currently have no answer.

SirFartALot
  • 1,215
  • 5
  • 25
  • As explained in this link - https://stackoverflow.com/a/28147706/6407858 the `getAndSet` atomically sets the new value and return the new value. It uses a compare and Swap mechanism to perform this which ensures atomicity. Therefore, IMHO you assertion that `aBoolean.getAndSet(!aBoolean.get())` is not atomic is incorrect.You can have a look at docimentation also. – Yug Singh Apr 25 '19 at 08:49
  • 2
    @YugSingh there are _two_ individual actions here, each, separately (`!aBoolean.get()` and `aBoolean.getAndSet`) are atomic; combined they are not. this is also called the anti-pattern "check than act" – Eugene Apr 25 '19 at 11:13
  • Nevertheless, your use of the atomic variable is not atomic. Your steps are (1) get currennt value of aBoolean, call this v1, (2) get current value of aBoolean, call this v2, and set value of aBoolean to !v1, (3) take action based on value v2. There's nothing that ensures v1 equals v2, and that's almost certainly the core of your problems. –  Apr 25 '19 at 11:15
  • 1
    When you added the 'lock' variable, where was the lock actually acquired? My advice: just use ``synchronized``` until the point at which you possess solid evidence that it is causing a performance issue. Using atomic variables for synchronization requires very careful analysis, and is very easy to get wrong. –  Apr 25 '19 at 11:24
  • Or replace the `AtomicBoolean` with an `AtomicInteger` and use `aInteger.getAndUpdate(i -> ~i) == 0` to test and flip it atomically. – Holger Apr 25 '19 at 13:12
  • @another-dave the lock was acquired and released inside the `call()` method of `Store` and `Fetch` class as - `lock.lock() and lock.unlock()`. Why I am not using `synchronized` as both `Fetch` and `Store` are two different classes and I need a mechanism to synchronize both on same object. So I thought when I need to create some lock object and pass it to both call functions of different classes to synchronize why not make use of `lock` utility of `java.util.concurrent package`. – Yug Singh Apr 25 '19 at 14:18
  • @Holger let me try and test using the same. Will let you know the results – Yug Singh Apr 25 '19 at 14:19