Request Serialized Threads

[gregw]

The example below is a very typical race that exists in many applications between a timeout and asynchronous processing:

@Override
protected void doGet(HttpServletRequest request,
                     HttpServletResponse response) throws IOException
{
    AsyncContext async = request.startAsync();
    PrintWriter out = response.getWriter();
    async.addListener(new AsyncListener() 
    {
        @Override
        public void onTimeout(AsyncEvent asyncEvent) throws IOException
        {
            response.setStatus(HttpServletResponse.SC_BAD_GATEWAY);
            out.printf("Request %s timed out!%n", request.getServletPath());
            out.printf("timeout=%dms%n ", async.getTimeout());
            async.complete();
        }
    });

    CompletableFuture<String> logic = someBusinessLogic();
    logic.thenAccept(answer ->
    {
        response.setStatus(HttpServletResponse.SC_OK);
        out.printf("Request %s handled OK%n", request.getServletPath());
        out.printf("The answer is %s%n", answer);
        async.complete();
    });
}

Because the handling of the result of the business logic may be executed by a non-container-managed thread, it may run concurrently with the timeout callback. The result can be an incorrect status code and/or the response content being interleaved. Even if both lambdas grab a lock to mutually exclude each other, the results are sub-optimal, as both will eventually execute and one will ultimately throw an IllegalStateException, causing extra processing and a spurious exception that may confuse developers/deployers.

The current specification of the asynchronous life cycle is the worst of both worlds for the implementation of the container. On one hand, they must implement the complexity of request-serialized events, so that for a given request there can only be a single container-managed thread in service(...), doFilter(...), onWritePossible(), onDataAvailable(), onAllDataRead()and onError(), yet on the other hand an arbitrary application thread is permitted to concurrently call the API, thus requiring additional thread-safety complexity. All the benefits of request-serialized threads are lost by the ability of arbitrary other threads to call the Servlet APIs.

Request Serialized Threads

The fix is twofold: firstly make more Servlet APIs immutable (as discussed #411) so they are safe to call from other threads; secondly and most importantly, any API that does mutate state should only be able to be called from request-serialized threads! The latter might seem a bit draconian as it will make the lambda passed to thenAccept in the example above throw an IllegalStateException when it tries to setStatus(int) or call complete(), however, there are huge benefits in complexity and correctness and only some simple changes are needed to rework existing code.

Any code running within a call to service(...), doFilter(...), onWritePossible(), onDataAvailable(), onAllDataRead() and onError() will already be in a request-serialized thread, and thus will require no change. It is only code executed by threads managed by other asynchronous components (e.g. the lambda passed to thenAccept() above) that need to be scoped. There is already the method AsyncContext.start(Runnable) that allows a non-container thread to access the context (i.e. classloader) associated with the request. An additional similar method AsyncContext.dispatch(Runnable) can be provided that not only scopes the execution but mutually excludes it and serializes it against any call to the methods listed above and any other dispatched Runnable. The Runnables passed may be executed within the scope of the dispatch call if possible (making the thread momentarily managed by the container and request serialized) or scheduled for later execution. Thus calls to mutate the state of a request can only be made from threads that are serialized.

To make accessing the dispatch(Runnable) method more convenient, an executor can be provided with AsyncContext.getExecutor() which provides the same semantic. The example above can now be simply updated:

@Override
protected void doGet(HttpServletRequest request,
                     HttpServletResponse response) throws IOException
{
    AsyncContext async = request.startAsync();
    PrintWriter out = response.getWriter();
    async.addListener(new AsyncListener() 
    {
        @Override
        public void onTimeout(AsyncEvent asyncEvent) throws IOException
        {
            response.setStatus(HttpServletResponse.SC_BAD_GATEWAY);
            out.printf("Request timed out after %dms%n ", async.getTimeout());
            async.complete();
        }
    });

    CompletableFuture<String> logic = someBusinessLogic();
    logic.thenAcceptAsync(answer ->
    {
        response.setStatus(HttpServletResponse.SC_OK);
        out.printf("The answer is %s%n", answer);
        async.complete();
    }, async.getExecutor());
}

Because the AsyncContext.getExecutor() is used to invoke the business logic consumer, then the timeout and business logic response methods are mutually excluded. Moreover, because they are serialized by the container, the request state can be checked between each, so that if the business logic has completed the request, then the timeout callback will never be called, even if the underlying timer expires while the response is being generated. Conversely, if the business logic result is generated after the timeout, then the lambda to generate the response will never be called. Because both of the tasks in this example call complete, then only one of them will ever be executed.

Note that the new APIs could be introduced in 6.0 without preventing the state APIs being called from other threads (perhaps they just give a warning the first time they are so called). The prevention of calling the APIs from arbitrary threads can be done in a 6.1 or 7.0 version of the spec.