Marek,
On 07/12/11 10:26, Marek Potociar wrote:
> Adding some of the evaluation comments from my emails earlier.
>
> On 12/06/2011 02:31 PM, Sergey Beryozkin wrote:
>> Hi,
>>
>> We have discussed various options to do with the way Filter signatures should look like. Simplicity and extensibility
>> have been two major topics during those discussions.
>>
>> Here is a brief overview of these options.
>>
>> 1. Filter implementation calls on FilterChain in order to indicate what it is willing to do with the current request:
>> let it continue, block it or suspend it. FilterChain returns an opaque interface instance such as NextAction which will
>> let the JAX-RS runtime decide what to do next.
>>
>> Example:
>>
>> public class MyFilter implements RequestFilter {
>>
>> public NextAction preFilter(FilterContext context) {
>>
>> if (stopProcessing) {
>> ResponseBuilder rb = context.getResponseBuilder();
>> return context.stop(rb.status(401).build())
>> } else if (suspendProcessing) {
>> return context.suspend(...);
>> } else {
>> RequestBuilder rb = context.getRequestBuilder();
>> return context.continue(rb.setSomething().build());
>> }
>>
>> }
>> }
>>
>>
>> Pros:
>> FilterContext is effectively a read-only instance, developers need to 'return' it as opposed to saving it at the
>> context itself.
> Keeps the call stack small (good because JVM does not support tail call optimization (TCO) which does not seem to
> change any time soon).
> Non-blocking, parallel or asynchronous filter execution strategies are possible and easy to add extensions. This
> enables high-performance implementations of execution chain leveraging the nature of multi-core environments.
> Bytecode-optimization-friendly design
The argument about the extensions is overloading it a bit. As if
following this pattern you are advocating is the only way to deal with
extensions... - may be we should kick off a new thread re the extensions.
Now, I'm for the option 2 at the moment, but option 1 can become a bit
become reasonable IMHO if we can get rid of NextAction and add something
less opaque instead.
What about turning existing FilterAction from enum to interface which
will have a single getter returning Enum:
interface FilterResponse {
enum Action {
NEXT, STOP, SUSPEND
}
Action getAction();
}
Comments ?
Cheers, Sergey
>> Cons:
>> The opaqueness of NextAction may cause questions like 'why it is here if we can't do anything with it' ?
>>
>> 2. Filter implementation calls on FilterChain in order to indicate what it is willing to do with the current request:
>> let it continue, block it or suspend it. No response type is available in the signature
>>
>> Example:
>>
>> public class MyFilter implements RequestFilter {
>>
>> public void preFilter(FilterContext context) {
>>
>> if (stopProcessing) {
>> ResponseBuilder rb = context.getResponseBuilder();
>> context.stop(rb.status(401).build())
>> } else if (suspendProcessing) {
>> context.suspend(...);
>> } else {
>> RequestBuilder rb = context.getRequestBuilder();
>> context.continue(rb.setSomething().build());
>> }
>> }
>> }
>>
>> This may be possibly optimized/improved.
>>
>> Pros:
>> Developers are familiar with filter signatures having no response type, example servlet filters.
>> Cons:
>> I believe there's a concern about possible bugs (someone forgets to do final else for example) and the possibility of
>> having few empty returns.
> Looks like wrapping style, but is non wrapping, which is confusing. May lead to errors with users trying to do some
> post-processing logic after context.[actionMethod] is invoked.
>>
>> 3. Filter implementation only calls on FilterChain to get the information it needs in order to decide what to do with
>> the current request: let it continue or block it or suspend it. It returns Response to indicate the request is blocked;
>> this can be null meaning that the the request is OK to continue
>>
>> Example:
>>
>> public class MyFilter implements RequestFilter {
>>
>> public Response preFilter(FilterContext context) {
>>
>> if (stopProcessing) {
>> return context.getResponseBuilder().status(401).build();
>> } else if (suspendProcessing) {
>> context.suspend(...);
>> // possibly 'return null'
>> } else {
>> RequestBuilder rb = context.getRequestBuilder();
>> context.setRequest(rb.setSomething().build());
>> return null;
>> }
>> }
>> }
>>
>> Pros:
>> FilterContext represents the current request and the filter implementation only updates it when it has something to
>> change on this request; if it does not then it returns a blocking Response
> Simple to implement and sufficient for the current (basic) set of requirements.
>> Cons:
>> Returning 'null' is not the best pattern and some users may dislike it.
>> Concerns about the extensibility - FilterChain may offer an option to query for specific extensions.
> Returning a *response* from a *request* filter raises concerns about the design.
>>
>> 4. The current trunk version
>>
>> public class MyFilter implements RequestFilter {
>>
>> public FilterAction preFilter(FilterContext context) {
>>
>> if (stopProcessing) {
>> ResponseBuilder rb = context.getResponseBuilder();
>> context.setResponse(rb);
>> return FilterAction.STOP;
>> } else if (suspendProcessing) {
>> context.suspend(...);
>> return FilterAction.SUSPEND;
>> } else {
>> RequestBuilder rb = context.getRequestBuilder();
>> context.setRequest(rb.setSomething().build());
>> return FilterAction.NEXT;
>> }
>> }
>> }
>>
> Addition from Bill Burke:
>>
>> 5. Any implementation (1-4), but its implemented as one java call stack, rather than a for-loop. FilterContext would
>> have a proceed() method which a Request(or Response) filter would have to execute to proceed to the next filter in the
>> chain.
>>
>> Pros:
>> - Consistent with MBW/MBR handler
>> - Previously executed filters can perform logic based on the state of the filter chain, i.e. If an exception is thrown,
>> catch it and maybe even recover. If suspended or an exception thrown, maybe they need to do cleanup
>> - You don't ever need to add another callback method to the RequestFilter interface if down the line users want to be
>> notified of exceptions, suspensions, etc...
>>
>> Cons:
>> - You really aren't wrapping around anything.
> - large filter chains produce large stacks (no TCO in Java = performance penalty)
> - filters are supposed to be stateless, so it's not clear what should be part of the clean-up or reconciliation tasks
> - mixes filter and exception mapper concepts together, adds one more way of doing the same thing
> - makes async, parallel and non-blocking execution strategies very hard to implement
>>
>> One extra consideration about the extensions. Ultimately, the job of the filter is to block or modify the request.
>
> That's not complete. Observer is also a type of filter.
>
>> Suspending the request is about getting the filter prepared for what it is supposed to do: block or modify the request.
>> Perhaps that may let us decide that making suspending the requests possible from within filters should not affect
>> FilterContext except for introducing an optional generic extension query handler
>
> Let's assume you have a following request filter chain:
>
> Observer filter F1 reads content
> Observer filter F2 reads header H1
> Observer filter F3 reads header H2, involves expensive calls (e.g. I/O to external systems)
>
> Suppose you have a request like this that is coming on a slow connection:
>
> GET /foo HTTP/1.1
> ... some headers ...
> H2
> ... some headers ...
> H1
> ... some headers ...
>
> ...content...
>
> Without any extensions the whole filter chain needs to block on I/O operations in F1 until the content is fully read and
> processed. This is a significant and obviously unnecessary performance penalty given the fact that all three filters
> could have been executed in parallel, using non-blocking I/O and by the time the F1 and F2 are finished it is likely
> that the expensive invocation of F3 has already finished too.
>
> Now I am not saying that we should address such scenarios right away or that we should expose our users to any complex
> implementation details around fork-join or any other execution strategy. But this is just to demonstrate what we may
> expect to deal with in the future, esp. if we, as implementors, want to leverage the parallel/async execution to gain
> performance boosts. That should help us to produce design flexible enough to support these types of scenarios. If all
> that we have to do right now is to convert an existing enum into an opaque interface and add bunch of action methods
> into the FilterContext, then I wonder why don't we just do it?
>
> Marek
>
>
>>
>> Thanks, Sergey
--
Sergey Beryozkin
Talend Community Coders
http://coders.talend.com/
Blog: http://sberyozkin.blogspot.com