yet another question on servlet thread safety

[J. Kevin Robbins]

I've read several dozen posts on this topic and it just gets more confusing. I've ordered a copy of Java Threads and I promise I'll read it twice when it arrives. In the meantime, please try to clear up some of my confusion.

If I understand (unlikely, but work with me here), each request to a servlet is a separate thread, therefore any object (other than instance variables) should be unique to each thread. So it seems to follow that I shouldn't need to synchronize on the creation of an object or the code within that object. How am I doing so far?

Let's do an example. Here's a servlet:

public class DataExportAjaxServlet extends HttpServlet {

    protected void processRequest(HttpServletRequest request, HttpServletResponse response)
            throws ServletException, IOException {

        response.setContentType("text/html;charset=UTF-8");
        PrintWriter out = response.getWriter();
        String action = request.getParameter("action");
        IAJAXHandler ah = null;

        if (!action.isEmpty()) {
            ah = DataExportAjaxFactory.getAjaxHandlerInstance(action);
        }
        if (ah != null) {
            String result = ah.performTask(request);
            out.print(result);
            out.flush();
            out.close();
        }
    }

So is it safe to say that each each object returned from the DataExportAjaxFactory is thread-safe or does it depend on the code with that object? This is an example of one of the objects that could be returned from the factory:

public class DataExportColumnHeadings implements IAJAXHandler {

    @Override
    public String performTask(HttpServletRequest request) {
        String optionName = request.getParameter("option");
        List<String> colNames = new ArrayList<String>();
        StoredProcedureOptions spInfo = StoredProcedureOptions.getInstance();
        StoredProcedureOptionsBean spBean = spInfo.getViewInfo(optionName);
        String daoNeeded = spBean.getDAOName();
        IDataExportDao dao = DataExportDaoFactory.getDaoInstance(daoNeeded);
        dao.setSql(spBean.getSqlString());
        StringBuilder colsAsHtml = new StringBuilder();

        colNames = dao.getColNames();
        for (String columnHeading : colNames) {
            colsAsHtml.append("<th>");
            colsAsHtml.append(columnHeading);
            colsAsHtml.append("</th>");
        }
        return colsAsHtml.toString();
    }
}

Note that it uses a StringBuilder and an ArrayList, neither of which is inherently thread-safe, but since each object is created by a single thread processed by the servlet, I'm confused as to whether this code is thread-safe.

[Rob Spoor]

Jk Robbins wrote:So is it safe to say that each each object returned from the DataExportAjaxFactory is thread-safe or does it depend on the code with that object?

It depends on the object, and what is returned. If DataExportAjaxFactory.getAjaxHandlerInstance(action) always returns a new object, then this object is used in only one thread and there's no need for synchronization. If you cache objects then the object may be used in multiple threads. There are again two options; the performTask method only uses local variables which means that call is thread safe, or it doesn't and you need to add synchronization. Seeing the code of your performTask method this way of thinking goes a bit deeper with StoredProcedureOptions objects, IDataExportDao objects, etc. I'm guessing this goes all the way to the database server itself.

Note that it uses a StringBuilder and an ArrayList, neither of which is inherently thread-safe, but since each object is created by a single thread processed by the servlet, I'm confused as to whether this code is thread-safe.

Local variables are never a problem unless you use them in other threads, which you don't here.

[J. Kevin Robbins]

Rob Spoor wrote:

Jk Robbins wrote:So is it safe to say that each each object returned from the DataExportAjaxFactory is thread-safe or does it depend on the code with that object?

It depends on the object, and what is returned. If DataExportAjaxFactory.getAjaxHandlerInstance(action) always returns a new object, then this object is used in only one thread and there's no need for synchronization. If you cache objects then the object may be used in multiple threads.

The factory always returns a new object, so it looks like I'm safe so far. But it also sounds like thread safety can be "lost" at other levels depending on the code of the objects instantiated and their use of instance variables and/or caching. Now I wonder if I've broken the thread-safety in the DAO because of the sql instance variable here:

public abstract class StoredProcedureDao implements IDataExportDao {

    private final static Logger LOGGER = Logger.getLogger(StoredProcedureDao.class.getName());
    protected String sql = "";

    public void setSql(String sql) {
        this.sql = sql;
    }

    public abstract List<String> getColNames();

    public List<List<String>> getResults(Map<String, String> parameters) {
        List<List<String>> rows = new ArrayList<List<String>>();
        List<String> parameterList = new ArrayList<String>(parameters.values());
        DataSourceHolder dsh = DataSourceHolder.getInstance();
        Connection conn = dsh.getConnection();
        ResultSet rset = null;
        CallableStatement cs = null;
        try {
            cs = conn.prepareCall(sql);
            for (int i = 0; i < parameterList.size(); i++) {
                cs.setString(i + 1, parameterList.get(i));
            }
            rset = cs.executeQuery();
            ResultSetMetaData rsetmd = rset.getMetaData();
            int numCols = rsetmd.getColumnCount();
            while (rset.next()) {
                List<String> cols = new ArrayList<String>();
                for (int i = 1; i <= numCols; i++) {
                    cols.add(rset.getString(i));
                }
                rows.add(cols);
            }
        }
        catch (SQLException sqle) {
            LOGGER.log(Level.SEVERE, null, sqle);
        }
        //<editor-fold defaultstate="collapsed" desc="finally block">
  
        return rows;
    }

Did I shoot myself in the foot with that instance variable? If so, I need to redesign this so I can pass the sql string to the method instead of setting it as a variable in the class object. The sql string is set to something like "{call MyStoredProcedure(?,?,?)}" and then the code on lines 21 and 22 set the parameters.

Thanks for the help. I think the fog is starting to clear but obviously I need to do some reading on this topic. As an aside, is there a reliable way to test for thread safety? For instance, can JUnit force multiple threads and try to break the code?

[Rob Spoor]

Again, if you always create a new StoredProcedureDao then you're safe. If not then this is indeed a cause for concern. Passing the SQL query as a parameter instead of having it be an instance field would then indeed solve this issue.

[J. Kevin Robbins]

Thank you, that helps a lot. Any thoughts on testing/confirming thread safety? I'm guessing that it comes down to understanding the code and the issues that cause problems with threading. I'm anxious to get my copy of Java Threads and really get my mind wrapped around this topic.