[PROPOSAL] Tomcat 4.0-beta API Change: Valve APIs

["Craig R. McClanahan" <Cr...@eng.sun.com>]

BACKGROUND:

The current API for Valves in Tomcat 4.0 implements the "Chain of
Responsibility" design pattern, but does so in a manner that requires individual
valves to be aware of each other in order to call the next valve in the chain.
(As Peter Donald colorfully puts it, this is the "Subversion of Control"
anti-pattern :-).  This design pattern was originally introduced on the
development track in Apache JServ a couple of years ago.

In addition, the Servlet Specification version 2.3 (Proposed Final Draft)
includes a new API called Filters, which can be used at the application level
for many tasks that one might use a Valve for inside of Catalna.  It would be
useful to have the two APIs that serve similar purposes be more aligned with
each other in their details.


THE CURRENT DESIGN PATTERN:

Currently, a Valve has the following fundamental API:

    public interface Valve {
        public Container getContainer(); <-- Owning container
        public void setContainer(Container container);
        public Valve getNext();
        public void setNext(Valve valve);
        public Valve getPrevious();
        public void setPrevious(Valve valve);
        public void invoke(Request request, Response response)
            throws IOException, ServletException;
    }

and a typical valve's invoke processing looks like this:

    ... preprocess the request ...
    getNext().invoke(request, response);
    ... postprocess the response ...

As you can see, a Valve must understand that it is part of a linked list, and
must deal with the case where you are the last Valve on the pipeline (so that
getNext() returns null).

There is an associated API, implemented by a Container that supports Valves:

    public interface Pipeline {
        public void addValve(Valve valve);
        public Valve findValves();    <-- Returns pointer to first one
        public void removeValve(Valve valve);
    }


THE PROPOSED DESIGN PATTERN:

This is an adaptation of the pattern that Peter Donald proposed yesterday, to be
slightly more aligned with the Filter API:

    public interface Valve {    <-- similar to javax.servlet.Filter
        public Container getContainer();    <-- Many valves need this
        public void setContainer(Container container);
        public void invoke(Request request, Response response,
            Pipeline pipeline) throws IOException, ServletException
    }

and the typical usage pattern in invoke() would be

    ... preprocess the request ...
    pipeline.invoke(request, response);
    ... postprocess the response ...

where the pipeline would manage the details of where the current request is in
the set of Valves to be processed, handles the "falling off the end" problem,
and so on.

Pipeline would become an interface describing a separate (from the Container)
object, which can therefore be reused by different Container implementations:

    public interface Pipeline {    <-- similar to javax.servlet.FilterChain
        public Container getContainer();
        public void setContainer(Container container);
        public void addValve(Valve valve);
        public void removeValve(Valve valve);
        public void invoke(Request request, Response response)
            throws IOException, ServletException
    }

Note that implementing the invoke() method here needs to be sensitive to the
fact that multiple requests could be at different points in the same pipeline at
the same time.  This can be dealt with by keeping the current position in a
ThreadLocal variable internally, or by creating pools of Pipeline objects that
represent the set of Valves to use for a particular Container (since they are
always the same), and have the Pipeline instances used by one thread at a time.
The API contract would need to define which way it works, so that Container
implementations can take the appropriate actions.


IMPACT OF CHANGE:

The following changes would be required inside the Tomcat 4.0 source repository
(I would be willing to undertake this, if the proposal is approved, unless some
other committer would like to volunteer instead):

* Rewrite the existing Pipeline and Valve interfaces

* Create standard Pipeline implementation class(es) that
  can be used by all containers, including support for pooling
  if that approach is selected.

* Modify all existing Container and Valve implementations to
  utilize the new APIs.  In most cases, these changes would
  be simplifications in the common ContainerBase and ValveBase
  classes, although each individual Valve would need to have
  its invoke() method signature and the way it invokes the next
  Valve in the pipline updated to the new APIs

* Release of a second beta that incorporates this change, to allow
  regression testing and make sure nothing got broken along the way.

* After completion, forward port to the 4.1 repository to maintain
  compatibility.


RECOMMENDATION:

I recommend that we do this change, and do it in 4.0 (rather than waiting for
4.1).  The revised design pattern is cleaner and easier for Valve implementors,
and doing this in 4.0 would avoid an incompatible API change between 4.0 and
4.1.

What do you think?

Craig

Re: [PROPOSAL] Tomcat 4.0-beta API Change: Valve APIs

[Remy Maucherat <re...@betaversion.org>]

Quoting "Craig R. McClanahan" <Cr...@eng.sun.com>:

> THE PROPOSED DESIGN PATTERN:
> 
> This is an adaptation of the pattern that Peter Donald proposed
> yesterday, to be
> slightly more aligned with the Filter API:
> 
>     public interface Valve {    <-- similar to javax.servlet.Filter
>         public Container getContainer();    <-- Many valves need this
>         public void setContainer(Container container);
>         public void invoke(Request request, Response response,
>             Pipeline pipeline) throws IOException, ServletException
>     }
> 
> and the typical usage pattern in invoke() would be
> 
>     ... preprocess the request ...
>     pipeline.invoke(request, response);
>     ... postprocess the response ...
> 
> where the pipeline would manage the details of where the current request
> is in
> the set of Valves to be processed, handles the "falling off the end"
> problem,
> and so on.
> 
> IMPACT OF CHANGE:
> 
> The following changes would be required inside the Tomcat 4.0 source
> repository
> (I would be willing to undertake this, if the proposal is approved,
> unless some
> other committer would like to volunteer instead):
> 
> * Rewrite the existing Pipeline and Valve interfaces
> 
> * Create standard Pipeline implementation class(es) that
>   can be used by all containers, including support for pooling
>   if that approach is selected.
> 
> * Modify all existing Container and Valve implementations to
>   utilize the new APIs.  In most cases, these changes would
>   be simplifications in the common ContainerBase and ValveBase
>   classes, although each individual Valve would need to have
>   its invoke() method signature and the way it invokes the next
>   Valve in the pipline updated to the new APIs
> 
> * Release of a second beta that incorporates this change, to allow
>   regression testing and make sure nothing got broken along the way.
> 
> * After completion, forward port to the 4.1 repository to maintain
>   compatibility.
> 
> 
> RECOMMENDATION:
> 
> I recommend that we do this change, and do it in 4.0 (rather than
> waiting for
> 4.1).  The revised design pattern is cleaner and easier for Valve
> implementors,
> and doing this in 4.0 would avoid an incompatible API change between 4.0
> and
> 4.1.
> 
> What do you think?

+1.
Forget about my other email : this one clarified everything.

Remy