Finding potential thread safety issues and race conditions in my multithreading code

I am working on a project in which I have two tables in a different database with different schemas. So that means I have two different connection parameters for those two tables to connect using JDBC-

Let's suppose below is the config.property file.

NUMBER_OF_THREADS: 10
TOTAL_RUNNING_TIME: 2
SLEEP_TIME: 200
RANGE_VALID_USER_ID: 1-1000
RANGE_NON_EXISTENT_USER_ID: 10000-50000
PERCENTAGE_VALID_USER_ID: 95
FLAG_VALIDATE_DATA: false
FLAG_STOP_PROGRAM: false
TABLES: table1 table2

#For Table1
table1.url: jdbc:mysql://localhost:3306/garden
table1.user: gardener
table1.password: shavel
table1.driver: jdbc-driver
table1.percentage: 80
table1.columns: COL1,COL2,COl3,Col4,COL5

#For Table2
table2.url: jdbc:mysql://otherhost:3306/forest
table2.user: forester
table2.password: axe
table2.driver: jdbc-driver
table2.percentage: 20
table1.columns: COL10,COL11,COl12,Col13,COL14

Now from the above property files it means-

• Number of Threads is 10

• TOTAL_RUNNING_TIME means each thread will run for 2 minutes

• Each thread will sleep for 200 milliseconds before making any other request

• Range of valid User Id from which each thread will pick the id's depending on the percentage. Like 95% of each thread will pick Valid Id's and remaining 5% it will pick Non Existent User Id.

• A boolean flag to validate the data.

• A boolean flag to stop the program in case of any exceptions.

Below method will read the above config.properties file and make a TableConnectionInfo object for each tables.

```
private static void readPropertyFile() throws IOException {

prop.load(LnPRead.class.getClassLoader().getResourceAsStream("config.properties"));

threads = Integer.parseInt(prop.getProperty("NUMBER_OF_THREADS").trim());
durationOfRun = Long.parseLong(prop.getProperty("TOTAL_RUNNING_TIME").trim());
sleepTime = Long.parseLong(prop.getProperty("SLEEP_TIME").trim());
flagValidateData = Boolean.parseBoolean(prop.getProperty("FLAG_VALIDATE_DATA").trim());

-
It looks thread-safe for me, I've not found any concurrency-related issue except that you might want to to use a ThreadLocalRandom or use separate SecureRandom instances with a ThreadLocal for performance reasons. See also. Is SecureRandom thread safe?

-

boolean valid = true;

Instead of this local variable you could return immediately if you know the result:

public boolean isJSONValid(final String str, final int id) {
    try {
        final JSONObject obj = new JSONObject(str);
        final JSONArray data = obj.getJSONArray("lv");
        final int n = data.length();

        for (int i = 0; i < n; ++i) {
            final JSONObject lv = data.getJSONObject(i);
            JSONObject v = lv.getJSONObject("v");
            if (v.getInt("userId") != id) {
                return false;
            }
        }
    } catch (JSONException ex) {
        return false;
    }
    return true;
}

-
The following fields could be local variables in run:

private Connection[] dbConnection = null;
private PreparedStatement preparedStatement = null;
private ResultSet rs = null;
private HashMap tableStatement = new HashMap();

Effective Java, Second Edition, Item 45: Minimize the scope of local variables

-
The null assignment is unnecessary here (in both cases):

if (preparedStatement != null) {
    try {
        preparedStatement.close();
        preparedStatement = null;
    } catch (SQLException e) {
        LOG.error("Threw a SQLException in finally block of prepared statement " + getClass().getSimpleName(),
                e);
    }
}

for (Connection con: dbConnection) {
    if (con != null) {
        try {
            con.close();
            con = null;
        } catch (SQLException e) {
            LOG.error("Threw a SQLException in finally block of dbConnection " + getClass().getSimpleName(), e);
        }
    }
}

It only nulls a local variable/parameter which will be destroyed anyway at the end of the block/method. I guess you wanted to null the field, for that you need a this. prefix but as above, the fields could be local variables in run, so it's unnecessary.

-
A guard clause could make the second loop flatten:

for (Connection con: dbConnection) {
    if (con == null) {
        continue;
    }
    try {
        con.close();
    } catch (SQLException e) {
        LOG.error("Threw a SQLException in finally block of dbConnection " + getClass().getSimpleName(), e);
    }
}

-

LOG.error("Threw a SQLException in finally block of prepared statement " 
    + getClass().getSimpleName(), e);

If you are using Logback or Log4j the getClass().getSimpleName() is unnecessary, you can set in the log4j.xml or logback.xml to log the class name for every log statement.

-
Instead of System.nanoTime(), you could use a stopwatch class, like Guava Stopwatch or Apache Commons StopWatch).

-
You could extract out the increment part of this method for higher abstraction level:

private static void handleException(String cause, boolean flagTerminate) {
    LOG.error(cause);

    AtomicInteger count = exceptionMap.get(cause);
    if (count == null) {
        count = new AtomicInteger();
        AtomicInteger curCount = exceptionMap.putIfAbsent(cause, count);
        if (curCount != null) {
            count = curCount;
        }
    }
    count.incrementAndGet();

    if (flagTerminate) {
        System.exit(1);
    }
}

It's easier to read, it's easier to get an overview what the method does (without the details) and you can still check them if you are interested in. Furthermore, you need to read and understand less code (not the whole original method) if you want to modify just a small part of it.

private static void handleException(String cause, boolean flagTerminate) {
    LOG.error(cause);
    incrementExceptionCount(cause);
    if (flagTerminate) {
        System.exit(1);
    }
}

private static void incrementExceptionCount(final String cause) {
    ...
}

-
I guess you could use here the same structure with AtomicLongs:

private static void incrementExceptionCount(final String cause) {
    AtomicInteger previousValue = exceptionMap.putIfAbsent(cause, new AtomicInteger());
    if (previousValue != null) {
        previousValue.incrementAndGet();
    }
}

You could also extract a similar method for AtomicLongs, since currently it's duplicated in the code.

-
Anyway, instead of the ConcurrentHashMap structures I'd use Guava's AtomicLongMap. It was designed for these situations.

(Effective Java, 2nd edition, Item 47: Know and use the libraries The author mentions only the JDK's built-in libraries but I think the reasoning could be true for other libraries too.)

-
The logic in the method parameter is duplicated here.

```
} catch (ClassNotFoundException e) {
handleException(e.getCause() != null ? e.getCause().toString() : e.toString(), LnPRead.flagTerminate);
} catch (SQLException e) {
handleExc

StackExchange Code Review Q#23409, answer score: 6