jakartaee / jakartaee/servlet

Request Serialized Threads

Open
#412 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Java
Stars
325
Forks
112
PR merge metrics
No merged PRs in 30d

Description

The example below is a very typical race that exists in many applications between a timeout and asynchronous processing:
```java
@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 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:
```java
@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 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.

Contributor guide

Open the contributing guide

Research direction

Start by reviewing the AsyncContext API, especially start(Runnable), and the request-serialized lifecycle methods listed in the issue. Compare the proposed dispatch(Runnable) and getExecutor() semantics with the current asynchronous lifecycle. Done means reaching agreement on the API and versioning changes needed to serialize mutable request operations.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
api, backend
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.