cxf-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Polar Humenn <phum...@iona.com>
Subject Re: Proposal for chaning CXF Interceptor APIs. WAS: RE: When should we close the handlers in CXF?
Date Tue, 13 Feb 2007 01:10:56 GMT
Hi Jervis,

Comments in line.

Liu, Jervis wrote:
>   
>> -----Original Message-----
>> From: Polar Humenn [mailto:phumenn@iona.com]
>> Sent: 2007?2?13? 3:33
>> To: cxf-dev@incubator.apache.org
>> Subject: Re: Proposal for chaning CXF Interceptor APIs. WAS: 
>> RE: When should we close the handlers in CXF?
>>
>>
>> So, if handleFault is removed, then there is no way to discern if an 
>> interceptor threw a fault during processing?
>>
>> So, you are saying that a Fault may not be thrown in onTermination()?
>>
>> I don't agree. The purpose of onTermination in this proposal 
>> is still to 
>> "handle" the message, which allows semantically for throwing 
>> a handling 
>> fault. Fault handling should unwind through *all* the operations that 
>> got it there, so that things may indeed be *unwound*.
>>
>> I'll admit my interleaving example is a bit contrived, but never the 
>> less, in a system as dynamically configurable as this interceptor 
>> architecture is now, anything is possible, and  I submit that the 
>> process to get it done is counter intuitive and complex in this 
>> proposal. It is better to have a simple and clean semantics..
>>
>>     
>
> I agree that it would be a simpler semantic if we can have a chain that only contains
the calling of handleMessage(message) and only traverses on one direction. But a second thought
makes me to think about the trade off we have to make in order to achieve this: First we have
to write corresponding ending interceptors for these need a terminal action, agreed it is
not a bit deal to write more interceptors and more phases. Secondly and more importantly,
we have to make sure the ending interceptors are put in a proper place, this is where trick
plays. Its not that straightforward and easy to make mistakes to find a proper place in the
chain for ending interceptors. 
Actually, this is where the phases come into play. Surprisingly I like 
this architecture. I like this architecture especially over the CORBA 
interceptor architecture, as that had no idea of ordering at all.

The phase thing is somebody's shear brilliance. I wish I had thought of 
it. It actually sections out ordering semantics for interceptor 
execution. CORBA couldn't figure this out, so they PUNTED.

I say Fantastic! So, bloody-well use it! Don't pretend it doesn't exist. 
The only slight complication you speak of comes with interceptors that 
are placed in the same phase. However,  there is some notable, if not 
great mitigation strategy.

> Things can get even worse if we have too many interceptors in a same phase, I dont think
addBefore and addAfter can handle complicate ordering cases.
I think if you lay out the phases with forethought, and state 
requirements for each, I doubt that you'll really get more than one 
interceptor per phase. However, the capability remains for those who 
require it, and there is some mitigation for ordering among them.

If certain internal interceptors are required to be in special places, 
then make the phases they reside on "protected" so that none, but the 
designated "system" interceptors can be on that certain phase.
> As a side note, currently the way how interceptors order themselves is not ideal, as
you already noticed:  "As far as the phases go, as far as I can tell so far, that the amount
of phases and names of the phases are configurable to the particular interceptor chain, and
don't really have much "meaning" associated with them at the moment, except for the implicit
ordering and where some of the CXF internal interceptors are actually placed." Someone wants
to refactor this? ;-) Anyway, that will be another story. 
Well, yes. The semantics for each phase are only suggestive by name, but 
you may lay down by documentation certain semantics or invariants on 
phases to give developers a guide line of where they think they should 
place their interceptors. And you can "protect" certain phases only to 
hold certain known "system" interceptors.
> To summarize, without onComplete(or onTerminate) the semantics does look simpler as the
chain only flows on one direction, 
I am glad you agree on that point.
> but it leaves the burden of choosing a proper phase for ending interceptors to developers,

Yes, they are developers, they are burdened. (Not as burdened as the 
designers :))
> thus subjects to human errors and obviously more work to do for developers. 
I see no problem with developers having to understanding the system. And 
if human errors are what you are worried about, I doubt that you are 
going to stop those even in the other scenario.
> The semantic itself can not guarantee the terminal action in an ending interceptors is
called in a place that is symmetric to its corresponding starting interceptors. E.g., the
chain below
>
> Interceptor A -> Interceptor B.Staring -> Interceptor C.Staring -> Interceptor
C.Ending -> Interceptor B.Ending
>
> Developers are free to put interceptor C.Ending and Interceptor B.Ending anywhere, the
semantic does not enforce the ending interceptors are called in a symmetric position in the
chain corresponding to their starting interceptors.
Exactly! "Developers" are free to put the interceptors anywhere, 
presumably they know what they are doing. You have to rely on that. The 
straight forward line through the interceptor chain is simple to 
understand.
> For example, they can end up of having a chain:
>
> Interceptor A -> Interceptor B.Staring -> Interceptor C.Staring -> Interceptor
B.Ending -> Interceptor C.Ending
>
> This can be very wrong. Using onComplete released this burden, and I don't see a chain
with a traverse back phase on complete is that complicate in terms of semantics. 
But this could also be very right, if it was the developer's intention. 
There are reasons for this seemingly "unsymmetrical" approach. Take this 
scenario:

Interceptor A starts ready a message header
Interceptor B.starting places a collection object on the message.
Interceptor C.starting handles replaces B's object with a wrapper 
collection object

Other interceptors add to the collection object, which in turn modifies 
both B and C
objects simultaneously, of which C has an ongoing efficient encryption.

Interceptor B.ending closes its collection object, produces a integrity 
hash on the original added objects.

Interceptor C.ending closes its collection object using the hash 
produced by B creates a signature.

Interceptor D gets the collection object (which it assumes it knows 
exists after B.ending, but doesn't really know about C at all) and 
request that it be marshaled it into a message body

Interceptor A.ending writes the marshaled header and the marshaled 
message body to the wire.

You can see that C.starting needs to come after B.starting because the 
object wouldn't exist otherwise. You can also, C.ending needs to come 
after B.ending, because B's object needs to get the close before 
C.ending. Then and only then can C.ending assume it can close its object 
get the hash for the signature.

Now if B.ending threw a handling Fault, C.ending wont do anything 
because it never got called. Then on handleFault, B.ending can clean up 
its object. Interceptor C.starting can relinquish copy object. 
Interceptor B.starting can relinquish its object, Interceptor A, can 
note that its intended message never went out. etc.

This is all pretty straight forward, the fault handing works as it should

The only thing that has to be done is to be definitive with the Phases, 
and placing certain internal interceptors in certain phases.

Cheers,
-Polar
> As far as the phases go, as far as I can tell so far, that 
> the amount of 
> phases and names of the phases are configurable to the particular 
> interceptor chain, and don't really have much "meaning" 
> associated with 
> them at the moment, except for the implicit ordering and 
> where some of 
> the CXF internal interceptors are actually placed.  So, what would it 
> matter to have a couple more phases that are explicitly 
> created for the 
> CXF internal interceptors, such as 
> MessageSenderStartIntercpetor and a 
> MessageSenderEndInterceptor?
>
> Cheers,
> -Polar
>
> Dan Diephouse wrote:
>   
>>> Hi Polar, comments inline...
>>>
>>> On 2/12/07, Polar Humenn <phumenn@iona.com> wrote:
>>>       
>>>>         
>>>>> Agreed. It could be done via another interceptor, but 
>>>>>           
>> its a common
>>     
>>>> enough
>>>>         
>>>>> case that we'd like to make it simpler.
>>>>> On a related note I would like to see the method named 
>>>>>           
>>>> onTermination() -
>>>>         
>>>>> this would imply "on termination of the chain take this 
>>>>>           
>> action..." 
>>     
>>>> which
>>>>         
>>>>> would give interceptors a chance to close resources 
>>>>>           
>> associated with 
>>     
>>>> the
>>>>         
>>>>> message. I'm -1 on the current "postHandleMessage" method name.
>>>>>
>>>>>           
>>>> I would argue that you may make some of the "common" cases 
>>>>         
>> "simpler" to
>>     
>>>> a degree in the sense that both operations will be in the 
>>>>         
>> same class,
>>     
>>>> but it make the semantics much more complex in general, and less
>>>> efficient:
>>>>
>>>>      1. Many interceptors will have to implement 
>>>>         
>> onTermination() without
>>     
>>>> a need for it.
>>>>      2. It will get always get called.
>>>>
>>>> The only advantage of this approach is that interceptors 
>>>>         
>> may be able to
>>     
>>>> save some instance state between the two calls, like a 
>>>>         
>> reference to an
>>     
>>>> object. However, that can be done merely by two subclasses 
>>>>         
>> implementing
>>     
>>>> the interceptor interface inside a single class.
>>>>
>>>> Also, it complicates the fault handing, which hasn't yet 
>>>>         
>> been addressed.
>>     
>>>> For instance, what happens if a Fault is thrown in onTermination()?
>>>>     Does it unwind through handleFault()?
>>>>         
>>> No, handleFault is removed.
>>>
>>>    If so, in what direction?
>>>
>>>
>>> In reverse.
>>>
>>>    How many times? Once or twice? If possibly twice, which 
>>>       
>> first call
>>     
>>>> to handleFault called?
>>>>         
>>> handleFault is gone, and onTermination is called just once.
>>>
>>>    Does it unwind through the interceptor's handleFault() operation
>>>       
>>>> twice? On what run was it when it did?
>>>>         
>>> See above.
>>>
>>> I surmise that the current interceptor interface {handleMessage,
>>>       
>>>> handleFault) is adequate, and it was the doIntercept() and
>>>> doInterceptInSubChain() calls that kind of mucked up the 
>>>>         
>> cleanliness and
>>     
>>>> simplicity of the approach.
>>>>
>>>> Given that the proposal includes the eliminatation 
>>>>         
>> doIntercept() and
>>     
>>>> doInterceptInSubChain() you are going to have to the same 
>>>>         
>> amount of work
>>     
>>>> to current interceptors that use doIntercept and 
>>>>         
>> doInterceptInSubChain:
>>     
>>>> You will have to split the single handleMessage() that into a "save
>>>> state on the message" so that handleMessage and onTermination() may
>>>> communicate properly. However, this is the same amount of 
>>>>         
>> work you need
>>     
>>>> to do to create two separate interceptors using 
>>>>         
>> handleMessage calls.
>>     
>>>> Also, for example. let's say you require functionality 
>>>>         
>> that needs to be
>>     
>>>> interleaved between the handleMessage and onTermination() 
>>>>         
>> calls of one
>>     
>>>> interceptor (call it A). You will need two interceptors B 
>>>>         
>> and C as you
>>     
>>>> will not be able to get by with one. For example, 
>>>>         
>> interceptor B will
>>     
>>>> have a potent handleMessage() that goes AFTER interceptor 
>>>>         
>> A, and limp
>>     
>>>> onTermination() call. Inteceptor C must get installed 
>>>>         
>> BEFORE interceptor
>>     
>>>> A with a limp handleMessage() and a potent onTermination() 
>>>>         
>> call. I say
>>     
>>>> installing interceptor C before interceptor A is a counter 
>>>>         
>> intuitive
>>     
>>>> approach.
>>>>         
>>> I'm not sure I follow your scenario. Do you have a concrete 
>>>       
>> use case 
>>     
>>> in mind
>>> here? I think all the current uses of 
>>>       
>> doIntercept(inSubChain) can be 
>>     
>>> handled
>>> by the simple flow of calling handleMessage when moving 
>>>       
>> forward and then
>>     
>>> calling onTermination in reverse on all the interceptors 
>>>       
>> that have been
>>     
>>> executed.
>>>
>>> A simple linear installation of interceptors is clearer, 
>>>       
>> more efficient,
>>     
>>>> and has simple already defined fault handling.
>>>>         
>>> This isn't simply about creating a new class, its about 
>>>       
>> creating new 
>>     
>>> phases
>>> as well. Lets take the MessageSenderInterceptor for 
>>>       
>> example. Following 
>>     
>>> your
>>> approach we need two classes, but we also need additional phases at 
>>> the end
>>> of the chain which mirror the start phases. This is very not ideal 
>>> IMO. Its
>>> much simpler to call handleMessage() throughout the chain, 
>>>       
>> and then call
>>     
>>> onTermination() in reverse order once the chain is done. If a fault 
>>> occurs
>>> in the chain, onTermination is only called from that point back.
>>>
>>> - Dan
>>>
>>>       
>>     


Mime
View raw message