Mutualising multi-threaded calls to EJB from Servlet

I have a Servlet that makes an EJB call to a backing server which takes about a second to return some data that changes reasonably regularly.

I can't cache the data on the servlet-side, so I have decided to put the call to the EJB into a separate thread and use a Future to get the data as late as possible. There is no reason why an invocation of a servlet concurrent to an existing invocation can't use the data returned from the first request to the EJB. For instance, if there is already a thread waiting for the data from the EJB, don't make another request; use the data from the first request.

```
public MyServlet extends HttpServlet {

//this lock protects all access to currentRosterRequest
private static final ReentrantReadWriteLock LOCK = new ReentrantReadWriteLock();

private RosterEJB rosterEJB=...;

//the executor service that will process the roster request
private static final ExecutorService ROSTER_FETCH_EXECUTOR_SERVICE = Executors.newSingleThreadExecutor(new ThreadFactory() {
@Override
public Thread newThread(Runnable r) {
return new Thread(r, "roster-fetch");
}
});

//the roster request task - this never changes, it never needs parameters
private static final Callable> ROSTER_FETCH_TASK = new Callable>() {

@Override
public ArrayList call() throws Exception {
try {
return rosterEJB.getRoster();
} finally {
//one way or another the roster request has terminated
LOCK.writeLock().lock();
try {
//clear out the static variable
MyServlet.currentRosterRequest = null;
} finally {
LOCK.writeLock().unlock();
}
}
}
};

//the currently active request
private static Future> currentRosterRequest = null;

public void doGet(...) {

//obtain or create the current roster request
final Future> localRosterRequest = getLocalRosterRequest();

The concurrency looks complete to me in the sense that I cannot see any places it can fail, or block significantly. The static LOCK and Future make sense. The getLocalRosterRequest itself is technically correct in the sense that it returns the right results, without logic problems. There is no need for the double-check locking though. It saves nothing, and solves nothing, other than creating two separate lock points.

Additionally, methods should not return concrete-types when interface-types would be available. Your method (and Future should be of type List and not ArrayList

Your code pulls the writeLock() from the LOCK, and locks on that. There are only two places where you use LOCK (one in the call, the other in the get). Neither of them need a sub-lock of the LOCK, they can both operate on the LOCK itself.

Your code would be simpler, and slightly faster, as simply:

private Future> getLocalRosterRequest() {
        LOCK.lock();
        try {
            if (currentRosterRequest == null) { //if there isn't a current request, create a new one
                currentRosterRequest = ROSTER_FETCH_EXECUTOR_SERVICE.submit(ROSTER_FETCH_TASK);
            }
            return currentRosterRequest; //return a reference to the current request
        } finally {
            LOCK.unlock();
        }
  }

and then, in the call method, you would have:

try {
          return rosterEJB.getRoster();
      } finally {
          //one way or another the roster request has terminated
          LOCK.lock();
          try {
              //clear out the static variable
              MyServlet.currentRosterRequest = null;
          } finally {
              LOCK.unlock();
          }
      }

StackExchange Code Review Q#54635, answer score: 3