Servlets + injection -multithreading issues

Question

So for a school project we created a site where a user could submit a report on underwater life etc. We used simple dependency injection (javax.inject) and an error checking pattern as follows :

ReportService.java

public interface ReportService {
    public static enum ReportServiceErrorsENUM {
        DB_FAILURE, WRONG_COORD // etc
    }
    public Set<ReportServiceErrorsENUM> getLastErrors();
    public int addNewReport(Report report);
}

ReportServiceImpl.java

public class ReportServiceImpl implements ReportService {

    private Set<ReportServiceErrorsENUM> lastErrors;
    private @Inject ReportDAO reportDAO;

    @Override
    public Set<ReportServiceErrorsENUM> getLastErrors() {
        return this.lastErrors;
    }

    @Override
    public int addNewReport(Report report) {
        lastErrors= new HashSet<ReportServiceErrorsENUM>();//throw away previous errors
        UserInput input = report.getUserInput();
        if (input.getLatitude() == null) {
            addError(ReportServiceErrorsENUM.WRONG_COORD);
        }
        // etc etc
        if (reportDAO.insertReport(report) != 0) {
            // failure inserting the report in the DB
            addError(ReportServiceErrorsENUM.DB_ERROR);
        }
        if (lastErrors.isEmpty()) // if there were no errors
            return EXIT_SUCCESS; // 0
        return EXIT_FAILURE;  // 1
    }
}

SubmitReportController.java

@WebServlet("/submitreport")
public class SubmitReportController extends HttpServlet {
    private static final long serialVersionUID = 1L;
    private @Inject ReportService reportService;

    @Override
    protected void doPost(HttpServletRequest request,
    HttpServletResponse response) throws ServletException, IOException {
        Report report = new Report();
        // set the report's fields from the HttpServletRequest attributes
        if(reportService.addNewReport(report) == ReportService.EXIT_FAILURE) {
            for(ReportServiceErrorsENUM error : reportService.getLastErrors())
                // display the errors etc
        } else {
            // display confirmation
        }
    }
}

The idea is that the Servlet controller calls the service (which is injected) then checks the services' return value and calls getLastErrors() on the service if there was an error - to inform the user what went wrong etc. Now I just came to realize that this is not thread safe - the @Inject'ed ReportService (reportService) will be shared by all threads using the servlet

1. Is it (crosses fingers) ?
2. How could one improve on this error mechanism ?

Thanks

Answer 1

typically for servlets you'd want to keep those variables (generally called "state") in some container-managed context. i'd move these errors to the request scope - that way they're stored on the request object (conceptually) and any servlet/jsp/whatever working on that same request can see/edit them. different requests mean different data storage.

example code for using the request scope from a servlet can be found here: http://www.exampledepot.com/egs/javax.servlet/State.html

Answer 2

Your design is neither thread-safe nor ready to server multiple users. It is not thread safe because several users (browsers) can hit the servlet at the same time and in turn access lastErrors set concurrently. (Yes, there is only one instance of servlet and your service).HashSet which you use is not thread safe.

Also if two different people try to use the same application, they will overwrite and have access to reports (errors) submitted by each other. In other words there is global state shared among all users while there should have been a state per user/session.

By fixing the second issue (I gave you a tip: use HTTPSession) you are unlikely to see the first issue. This is because it is rather rare to see concurrent access to the same session. But it's possible (concurrent AJAX requests, two browser tabs). Keep that in mind, but there are more important issues to solve now.