How to identify non thread-safe code in a multi-threaded environment?

Question

I have designed and implemented a simple webstore based on traditional MVC Model 1 architecture using pure JSP and JavaBeans (Yes, I still use that legacy technology in my pet projects ;)).

I am using DAO design pattern to implement my persistence layer for a webstore. But I am not sure if I have implemented the classes correctly in my DAO layer. I am specifically concerned about the QueryExecutor.java and DataPopulator.java classes (mentioned below). All the methods in both these classes are defined as static which makes me think if this is the correct approach in multithreaded environment. Hence, I have following questions regarding the static methods.

1. Will there be synchronization issues when multiple users are trying to do a checkout with different products? If answer to the above question is yes, then how can I actually reproduce this synchronization issue?
2. Are there any testing/tracing tools available which will actually show that a specific piece of code will/might create synchronization issues in a multithreaded environment? Can I see that a User1 was trying to access Product-101 but was displayed Product-202 because of non thread-safe code?
3. Assuming there are synchronization issues; Should these methods be made non-static and classes instantitable so that we can create an instance using new operator OR Should a synchronized block be placed around the non thread-safe code?

Please guide.

MasterDao.java

public interface MasterDao {
    Product getProduct(int productId) throws SQLException;
}

BaseDao.java

public abstract class BaseDao {
    protected DataSource dataSource;
    public BaseDao(DataSource dataSource) {
        this.dataSource = dataSource;
    }
}

MasterDaoImpl.java

public class MasterDaoImpl extends BaseDao implements MasterDao {

    private static final Logger LOG = Logger.getLogger(MasterDaoImpl.class);

    public MasterDaoImpl(DataSource dataSource) {
        super(dataSource);
    }

    @Override
    public Product getProduct(int productId) throws SQLException {
        Product product = null;
        String sql = "select * from products where product_id= " + productId;
        //STATIC METHOD CALL HERE, COULD THIS POSE A SYNCHRONIZATION ISSUE ??????
        List<Product> products = QueryExecutor.executeProductsQuery(dataSource.getConnection(), sql);
        if (!GenericUtils.isListEmpty(products)) {
            product = products.get(0);
        }
        return product;
    }

}

QueryExecutor.java

public final class QueryExecutor {

    private static final Logger LOG = Logger.getLogger(QueryExecutor.class);

    //SO CANNOT NEW AN INSTANCE
    private QueryExecutor() {
    }

    static List<Product> executeProductsQuery(Connection cn, String sql) {
        Statement stmt = null;
        ResultSet rs = null;
        List<Product> al = new ArrayList<>();

        LOG.debug(sql);

        try {
            stmt = cn.createStatement();
            rs = stmt.executeQuery(sql);
            while (rs != null && rs.next()) {
                //STATIC METHOD CALL HERE, COULD THIS POSE A SYNCHRONIZATION ISSUE ???????
                Product p = DataPopulator.populateProduct(rs); 
                al.add(p);
            }
            LOG.debug("al.size() = " + al.size());
            return al;
        } catch (Exception ex) {
            LOG.error("Exception while executing products query....", ex);
            return null;
        } finally {
            try {
                if (rs != null) {
                    rs.close();
                }
                if (stmt != null) {
                    stmt.close();
                }
                if (cn != null) {
                    cn.close();
                }
            } catch (Exception ex) {
                LOG.error("Exception while closing DB resources rs, stmt or cn.......", ex);
            }
        }
    }

}

DataPopulator.java

public class DataPopulator {

    private static final Logger LOG = Logger.getLogger(DataPopulator.class);

    //SO CANNOT NEW AN INSTANCE
    private DataPopulator() {
    }

    //STATIC METHOD DEFINED HERE, COULD THIS POSE A SYNCHRONIZATION ISSUE FOR THE CALLING METHODS ???????
    public static Product populateProduct(ResultSet rs) throws SQLException {
        String productId = GenericUtils.nullToEmptyString(rs.getString("PRODUCT_ID"));
        String name = GenericUtils.nullToEmptyString(rs.getString("NAME"));
        String image = GenericUtils.nullToEmptyString(rs.getString("IMAGE"));
        String listPrice = GenericUtils.nullToEmptyString(rs.getString("LIST_PRICE"));

        Product product = new Product(new Integer(productId), name, image, new BigDecimal(listPrice));
        LOG.debug("product = " + product);

        return product;
    }

}

Answer 1

Your code is thread-safe.

The reason, and the key to thread-safety, is your (static) methods do not maintain state. ie your methods only use local variables (not fields).

It doesn't matter if the methods are static or not.

Answer 2

Assuming there are synchronization issues; Should these methods be made non-static and classes instantitable so that we can create an instance using new operator

This won't help. Multiple threads can do as they please with a single object just as they can with a static method, and you will run into synchronization issues.

OR Should a synchronized block be placed around the non thread-safe code?

Yes this is the safe way. Any code inside of a synchronized block is guaranteed to have at most one thread in it for any given time.

Looking through your code, I don't see many data structures that could possibly be shared amongst threads. Assuming you had something like

public final class QueryExecutor {
    int numQueries = 0;

    public void doQuery() {
        numQueries++;
    }
}

Then you run into trouble because 4 threads could have executed doQuery at the same moment, and so you have 4 threads modifying the value of numQueries - a big problem.

However with your code, the only shared class fields is the logging class, which will have it's own thread-safe synchronization built in - therefore the code you have provided looks good.

Answer 3

There is no state within your code (no mutable member variables or fields, for example), so Java synchronisation is irrelevant.

Also as far as I can tell there are no database creates, updates, or deletes, so there's no issue there either.

There's some questionable practice, for sure (e.g. the non-management of the database Connection object, the wide scope of some variables, not to mention the statics), but nothing wrong as such.

As for how you would test, or determine thread-safety, you could do worse than operate your site manually using two different browsers side-by-side. Or create a shell script that performs automated HTTP requests using curl. Or create a WebDriver test that runs multiple sessions across a variety of real browsers and checks that the expected products are visible under all scenarios...