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&lt;ArrayList&lt;String&gt;&gt; ROSTER_FETCH_TASK = new Callable&lt;ArrayList&lt;String&gt;&gt;() {

      @Override
      public ArrayList&lt;String&gt; 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&lt;ArrayList&lt;String&gt;&gt; currentRosterRequest = null;

  public void doGet(...) {

    //obtain or create the current roster request
    final Future&lt;ArrayList&lt;String&gt;&gt; localRosterRequest = getLocalRosterRequest();

    //do other things

    //wait up to two seconds for data from the roster request
    final Boolean hasAccess = localRosterRequest.get(2, TimeUnit.SECONDS).contains(user.toLowerCase());

    //etc
  }

  private Future&lt;ArrayList&lt;String&gt;&gt; getLocalRosterRequest() {
    Future&lt;ArrayList&lt;String&gt;&gt; localRosterRequest;
    LOCK.readLock().lock();
    try {
        localRosterRequest = currentRosterRequest; //get a reference to the current request, if there is one
    } finally {
        LOCK.readLock().unlock();
    }
    if (localRosterRequest == null) { //if there wasn't a current request
        LOCK.writeLock().lock();
        try {
            if (currentRosterRequest == null) { //if there still isn't a current request, create a new one
                currentRosterRequest = ROSTER_FETCH_EXECUTOR_SERVICE.submit(ROSTER_FETCH_TASK);
            }
            localRosterRequest = currentRosterRequest; //get a reference to the current request
        } finally {
            LOCK.writeLock().unlock();
        }
    }
    return localRosterRequest; //return the reference to the current request
  }
}


(I have left the majority of the servlet out - the main thing I want reviewing is the method getLocalRosterRequest().)

My aims:


If there is a request already underway, don't create a new request, use the existing one.
If a new GET comes in when there is no request underway, create a new request, and make it available to subsequent callers.
Avoid deadlocks!


My questions:


Is this threadsafe? I can't see a way that threads could block.
Does it put the servlet container at risk? I'm using Tomcat 6
Is there a better way?
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&lt;String&gt; and not ArrayList&lt;String&gt;

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&lt;List&lt;String&gt;&gt; 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();
          }
      }