No object wrapper identity on dispatch

[gregw]

The Servlet specification allows for wrappers to do more than just override existing methods! In 6.2.2, the specification says that:

… the developer not only has the ability to override existing methods on the request and response objects, but to provide new API…

So the example above could introduce new API to access the original user principal:

public static class ForcedUserRequest extends HttpServletRequestWrapper 
{ 
    // ... getUserPrincipal & isUserInRole as above
    public Principal getOriginalUserPrincipal() 
    { 
        return super.getUserPrincipal(); 
    }
    public boolean isOriginalUserInRole(String role) 
    { 
        return super.isUserInRole(role); 
    } 
}

In order for targets to be able to use these new APIs then they must be able to downcast the passed request/response to the known wrapper type:

@WebServlet(urlPatterns = {"/servletB/*"})
public static class ServletB extends HttpServlet
{
    @Override
    protected void doGet(HttpServletRequest req, HttpServletResponse resp)
        throws ServletException, IOException
    {
        MyWrappedRequest myr = (MyWrappedRequest)req;
        resp.getWriter().printf("user=%s orig=%s wasAdmin=%b%n", 
            req.getUserPrincipal(), 
            myr.getOriginalUserPrincipal(),
            myr.isOriginalUserInRole("admin"));
    }
}

This downcast will only work if the wrapped object is passed through the container without any further wrapping, thus the specification requires “wrapper object identity”:

… the container must ensure that the request and response object that it passes to the next entity in the filter chain, or to the target web resource if the filter was the last in the chain, is the same object that was passed into the doFilter method by the calling filter. The same requirement of wrapper object identity applies to the calls from a Servlet or a filter to RequestDispatcher.forward or RequestDispatcher.include, when the caller wraps the request or response objects.

This “wrapper object identity” requirement means that the container is unable to itself wrap requests and responses as they are passed to filters and servlets. This restriction has, directly and indirectly, a huge impact on the complexity, efficiency, and correctness of Servlet container implementations, all for very dubious and redundant benefits:

Bad Software Components
In the example of ServletB above, it is a very bad software component as it cannot be invoked simply by respecting the signature of its methods. The caller must have a priori knowledge that the passed request will be downcast and any other caller will be met with a ClassCastException. This defeats the whole point of an API specification like Servlets, which is to define good software components that can be variously assembled according to their API contracts.

No Multiple Concerns
It is not possible for multiple concerns to wrap request/responses. If another filter applies its own wrappers, then the downcast will fail. The requirement for “wrapper object identity” requires the application developer to have total control over all aspects of the application, which can be difficult with discovered web fragments and ServletContainerInitializers.

Mutable Requests
By far the biggest impact of “wrapper object identity” is that it forces requests to be mutable! Since the container is not allowed to do its own wrapping within RequestDispatcher.forward(...) then the container must make the original request object mutable so that it changes the value returned from getServletPath() to reflect the target of the dispatch. It is this impact that has significant impacts on complexity, efficiency, and correctness:

• Mutating the underlying request makes the example implementation of isOriginalUserInRole(String) incorrect because it calls super.isUserInRole(String) whose result can be mutated if the target Servlet has a run-as configuration. Thus this method will inadvertently return the target rather than the original role.
• There is the occasional need for a target Servlet to know details of the original request (often for debugging), but the original request can mutate so it cannot be used. Instead, an ever-growing list of Request attributes that must be set and then cleared on the original request attributes, just in case of the small chance that the target will need one of them. A trivial forward of a request can thus require at least 12 Map operations just to make available the original state, even though it is very seldom required. Also, some aspects of the event history of a request are not recoverable from the attributes: the isUserInRolemethod; the original target of an include that does another include.
• Mutable requests cannot be safely passed to asynchronous processes, because there will be a race between the other thread call to a request method and any mutations required as the request propagates through the Servlet container (see the “Off to the Races” example below). As a result, asynchronous applications SHOULD copy all the values from the request that they MIGHT later need…. or more often than not they don’t, and many work by good luck, but may fail if timing on the server changes.
• Using immutable objects can have significant benefits by allowing the JVM optimizer and GC to have knowledge that field values will not change. By forcing the containers to use mutable request implementations, the specification removes the opportunity to access these benefits. Worse still, the complexity of the resulting request object makes them rather heavy weight and thus they are often recycled in object pools to save on the cost of creation. Such pooled objects used in asynchronous environments can be a recipe for disaster as asynchronous processes may reference a request object after it has been recycled into another request.

Unnecessary
New APIs can be passed on objects set as request attribute values that will pass through multiple other wrappers, coexist with other new APIs in attributes and do not require the core request methods to have mutable returns.

Conclusion
The “wrapper object identity” requirement has little utility yet significant impacts on the correctness and performance of implementations. It significantly impairs the implementation of the container for a feature that can be rendered unusable by a wrapper applied by another filter. It should be removed from Servlet 6.0 and requests passed in by the container should be immutable.

[gregw]

On Mon, 26 Jul 2021 at 21:50, @markt-asf Mark Thomas [email protected] wrote:
On 24/07/2021 03:18, @gregw Greg Wilkins wrote:

• Can we drop "Object Wrapper Identity"? That would be a breaking
  change, but we'd be able to fix more broken async applications in
  the process.

I don't know. The big question is to what extent do users wrap objects
in order to add functionality rather than to override existing
behaviour. To put it another way, how much stuff will this break?

If we go this route, do you anticipate a new API that would let users
add functionality that would be unaffected by any subsequent wrapping?

[gregw]

@markt-asf There are undoubtably some applications that use this "feature" that will be broken if we use it. But I believe (without any concrete evidence) that there are more broken applications using async that do no realize they are in a race when they access the path methods. We are likely to fix more applications than we break by removing this "feature".

The "feature" was always a bad idea, encourages bad design, can be broken with simple utility filters, etc.

Note that many applications that use this "feature" do so between filters and servlets, which we can continue to support it there. The problem only really comes when there is a RequestDispatcher.forward or async dispatch, that mutates the path methods and has different behaviours for methods like isUserInRole().

[gregw]

Consider the following asynchronous Servlet:

@WebServlet(urlPatterns = {"/async/*"}, asyncSupported = true)
@RunAs("special")
public static class AsyncServlet extends HttpServlet
{
    @Override
    protected void doGet(HttpServletRequest request, HttpServletResponse response)
        throws ServletException, IOException
    {
        AsyncContext async = request.startAsync();
        PrintWriter out = response.getWriter();
        async.start( () ->
        {
            response.setStatus(HttpServletResponse.SC_OK);
            out.printf("path=%s special=%b%n", 
                       request.getServletPath(), 
                       request.isUserInRole("special"));
            async.complete();
        });
    }
}

If invoked via a RequestDispatcher.forward(...), then the result produced by this Servlet is a race: will the thread dispatched to execute the lambda execute before or after the thread returns from the doGet method (and any applied filters) and the pre-forward values for the path and role are restored? Not only could the path and role be reported either for the target or caller, but the race could even split them so they are reported inconsistently. To avoid this race, asynchronous Servlets must copy any value that they may use from the request before starting the asynchronous thread, which is needless complexity and expense. Many Servlets do not actually do this and just rely on happenstance to work correctly.

This problem is the result of the start/complete lifecycle of asynchronous Servlets permitting/encouraging arbitrary threads to call the existing APIs that were not designed to be thread-safe. This issue is avoided if the request object passed to doGet is immutable and if it is the target of a forward, it will always act as that target.

[gregw]

I've created draft PR #413 to illustrate the changes I'm proposing. Specifically that we only remove Object Wrapper Indentify for dispatches that change the behaviour of request/response objects and that those behaviour changes persist even after the dispatch is complete.

I acknowledge that is will break some applications, but if we are unable to remove such bad features as this, then there is no future for the servlet-api, as other APIs will out compete us on: performance, simplicity of usage; robust applications etc. If we can't remove such legacy kruft now, then when? Will we forever by an racy asynchronous API because of this rarely used bad feature?

As another example of why Object Wrapper Identity is bad, consider the following evil Servlet, which when the target of an include can use the race described above to set a response header even when it is not allowed to.

@WebServlet(urlPatterns = {"/evil/*"})
public static class EvilIncludedServlet extends HttpServlet
{
    @Override
    protected void doGet(HttpServletRequest request, HttpServletResponse response)
        throws ServletException, IOException
    {
        async.start(() ->
        {
            while(true)
            {
                try
                {
                    response.setHeader("Not", "Allowed");
                    break;
                }
                catch(Exception e)
                {}
            }
        });
    }
}