detecting race condition using findbugs or another analysis tool

Question

Below bean is not thread-safe: method addIfNotExist is not synchronized, so it is possible that the same term gets added twice because of race condition. I annotated the class using JCIP annotation @ThreadSafe hoping FindBugs would find that the implementation is not thread-safe and flag it as an error, but it is not. Are there any tools that identify these type of errors in code base?

Methods addIfNotExist and isExist should be synchronized to make this bean thread-safe. Should isExist method be also synchronized?

package com.test;

import java.util.ArrayList;

import java.util.Collection;

import net.jcip.annotations.GuardedBy;

import net.jcip.annotations.ThreadSafe;

@ThreadSafe

public class Dictionary {

    @GuardedBy("this")
    public Collection<String> terms = new ArrayList<String>();

    public void addIfNotExist(final String input) {
        if (!this.terms.contains(input)) {
            this.terms.add(input);
        }
    }

    public boolean isExist(final String input){
        return this.terms.contains(input);
    }

    public void remove(final String input){
        this.terms.remove(input);
    }
}

Answer 1

It is tremendously difficult to write safe multi-threaded code that has any degree of complexity: this type of locking (using monitors) is fraught with all sorts of intermittent race conditions, deadlocks and livelock issues that often evades detection right up into promotion into production systems; if you can, consider using message passing, software transactional memory or persistent data structures instead.

FindBugs (or indeed any static analysis tools) can only go so far in detecting non-thread safe code: by their very definition, race conditions are time-sensitive - they require many execution runs to manifest, so static analysis fails in this respect because they don't run any code and only look for common code signatures. The best things IMHO to detect issues are:

• A second pair of eyes - rigorous code reviews with peers who are familiar with the code - goes along way in finding bugs that are not immediately obvious to the original author.

• Continuous integration & exhaustive automated tests that exercise multi-threadedness on a variety of hardware, and ruthlessly investigate any 'intermittent' test failures.

In answer to the second question, yes, all methods that make some reference to terms should be guarded by a synchronization monitor, regardless of whether it is a write or read operation; consider what happens if thread A calls remove("BOB") whilst thread B is calling isExists("BOB") when you don't have synchronization - thread A will be compacting the array list while thread B will be attempting to traversing it.

At best, you will not be able to determine result of isExists("BOB"), but it is entirely possible that B could intermittently throw an IndexOutOfBounds exception since the size of the array could have changed (i.e shrunk) as it was being traversed over.

Synchronize, and while you still can't be sure of the order in which the calls are made (due to the non-deterministic nature of scheduling), but at least you will be guaranteed that operations on terms will be atomic - that is, they are not being altered by something else whilst the current thread is running.

Answer 2

This is something that you can use at run-time (during automated unit tests or integration tests or what have you) to help find threading problems: IBM ConTest (Concurrency Testing)

ConTest description: "The ConTest technology is innovative and counter-intuitive. Specifically, ConTest systematically and transparently schedules the execution of program threads such that program scenarios which are likely to contain race conditions, deadlocks and other intermittemt bugs - collectively called synchronization problems - are forced to appear with high frequency. In doing so, ConTest dramtically improves the quality of testing and reduces development expense, as bugs are found earlier in the testing process. "