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 Thu, 15 Feb 2007 15:20:45 GMT
Liu, Jervis wrote:
> Hi I think I am convinced by Polar eventually as there are some cases the onComlete schema
can not handle very elegantly. Can we finalize the proposal before we switch the topic to
sth else like fault handling, better naming of phases etc? So we've had four proposals so
far, thanks for the contributions of DanD :-). 
>
> 1. OnComplete()
> 2. No OnComplete(), the chain is a flat one way traverse, developers need to write an
ending interceptor when a terminal action is needed
> 3. A variation of subchain proposed by Dan D.
> 4. XFire Serializer
>
> I would veto Option 3 first as it still uses subchain, and I really don't like the idea
of subchain. Between option 2 and option 4, I personally prefer 2, as I found 2 is easier
to understand than 4 in terms of execution path and semantics. Also 2's semantics is very
close to CORBA interceptor, guess most guys from the old CORBA world would feel very comfortable
with this. Thoughts? 
>
>   
I do have some thoughts, being an old CORBA guy, myself.  =-O I'm the 
guy who started the CORBA interceptor standard rolling (one of those bad 
security guys trying to tell the ORB guys what to do!).   O:-)

Here is as least something to think about.

Eoghan brought up an interesting example the other day when we were 
talking about issuing a 401 status code in HTTP on the server side.

My solution was to throw a Fault in the password checker interceptor. I 
found that throwing a fault would magically generate a Fault Message 
with status 500, and I would intercept that on the Out Fault Chain and 
change the status code to a 401 and provide the WWW-Authentication header.

Eoghan presented an example whereby the password checker interceptor, 
retrieved the Exchange, created a correlated outgoing message containing 
the 401, and then pulled a Conduit.send() and Conduit.close() on it 
(completely skipping the out interceptor chain). And he had to do an 
InterceptorChain.abort() to prevent the message from going on to the 
application.

I'm not sure which approach is better, because I think the message 
internals are more unwieldy for CXF.

In CORBA, it's a little more defined. Every incoming message is a 
Request for which there is always a Reply, whether that be a real reply 
or an exception. If the reply or exception came back from the 
application, it would unwind through the interceptors with a 
send_reply() or send_exception() call, respectively. Also, if a Request 
Interceptor threw an Exception, that exception would be turned into a 
CORBA Exception Reply and that reply would unwind back  through the 
interceptors that got it there, through send_exception() or send_other() 
depending on whether it was a CORBA typed exception or other exception.

On the client side, it is the same, requests go out send_request, and 
returns through receive_reply(), receive_exception(), receive_other(). 
If a send_request throws an Exception, it unwinds back through one of 
the receive calls.

In CXF, it doesn't appear that an in/out Message has the notion of a 
matching out/in message. (at least for the purpose of interceptors).

Interceptors do all the "replying" on the server side, and on the client 
side Interceptors do the correlation of incoming messages with 
previously sent outgoing messages.

So, therefore an interceptor needs to be able to reverse the chain as in 
throwing a Fault, or at least stop it, as in Eoghan's example when it 
decides to "short-circuit" an intended reply.

Cheers,
-Polar

>   
>> -----Original Message-----
>> From: Polar Humenn [mailto:phumenn@iona.com]
>> Sent: 2007?2?15? 3:03
>> To: cxf-dev@incubator.apache.org
>> Subject: Re: Proposal for chaning CXF Interceptor APIs. WAS: 
>> RE: When should we close the handlers in CXF?
>>
>>
>> Dan Diephouse wrote:
>>     
>>> Alrighty, I think I'm convinced given that Dan pointed out 
>>>       
>> some better
>>     
>>> syntax. I think we'd need to some convenience methods to
>>> AbstractPhaseInterceptor with a signatures like this:
>>>
>>> void addInterceptor(String phase, Interceptor i)
>>> void addInterceptor(String phase, String before, String after, 
>>> Interceptor
>>> i)
>>>
>>> Then it becomes
>>>
>>> addInterceptor(Phase.MARSHAL, new Interceptor() {
>>> ...
>>> });
>>>
>>> Now I think the important thing is to define some better phases. I 
>>> need to
>>> spend a little bit of time thinking about Andrea's email 
>>>       
>> and digging 
>>     
>>> through
>>> code before I really commit to anything though.
>>>
>>> Is everyone still ok with removing handleFault as well?
>>>
>>>       
>> Does that mean you are going to get rid of the unwind?
>>
>> I'm not in favor of removing handleFault();
>>
>> Not that I'm a committer or anything :-[ , but I do use CXF. 8-)
>>
>> I actually think handleFault may be improved. When the 
>> PhaseInterceptorChain implementation gets a fault from a 
>> handleMessage, 
>> before it does the unwind, it places the thrown Fault on the Message 
>> using message.setContent(Exception.class, fault).
>>
>> Why is the interceptor chain modifying the message? Again, this seems 
>> like a violation of a separation of concerns/control issue.
>>
>> The handleFault method should explicitly be given the fault, such as:
>>
>>     void handleFault(Message msg, Fault fault);
>>
>> Cheers,
>> -Polar
>>
>>
>>     
>>> - Dan
>>>
>>> On 2/14/07, Polar Humenn <phumenn@iona.com> wrote:
>>>       
>>>> Hi Dan,
>>>>
>>>> I'm glad I convinced you on one point that was rockin' my boat.  :)
>>>> Thanks.
>>>>
>>>> My only objection to the current and proposed scheme is 
>>>>         
>> the "reentrant"
>>     
>>>> interceptor chain execution scheme. This seems like a 
>>>>         
>> violation of a
>>     
>>>> control principle. Why should an interceptor actively control the
>>>> mechanism that is controlling the interceptors?
>>>>
>>>> I guess it's like a disk device driver deciding bus access 
>>>>         
>> affecting
>>     
>>>> other devices it doesn't know about. (maybe a bad 
>>>>         
>> example). Or a user
>>     
>>>> process manipulating the operating system process table 
>>>>         
>> deciding what
>>     
>>>> should processes to run next.
>>>>
>>>> Furthermore, I think the sole purpose of the "reentrant" 
>>>>         
>> model is so
>>     
>>>> that a particular interceptor in Phase A, can perform an 
>>>>         
>> operation X
>>     
>>>> after Phase B further down the chain, which seems 
>>>>         
>> confusing to me. I
>>     
>>>> would think action X would be executed in some phase 
>>>>         
>> *after* B, not in
>>     
>>>> Phase A.
>>>>
>>>> So, I have three points:
>>>>
>>>> Point 1: Lets say, if I put an interceptor in the position 
>>>>         
>> of the last
>>     
>>>> phase, shouldn't I expect that I am the last interceptor 
>>>>         
>> point, and the
>>     
>>>> last actions that are done are mine?
>>>>
>>>> Point 2: fault handling. Granted, the doIntercept() call 
>>>>         
>> returns false
>>     
>>>> instead of true if the chain was aborted or not completed 
>>>>         
>> yet, and maybe
>>     
>>>> the actions after that can take that into account, but *why* is it
>>>> *allowed* to do this? This interceptor should have *already* had
>>>> handleFault called, yet this interceptor is still in the 
>>>>         
>> handleMessage()
>>     
>>>> call, technically 'handling" the message. This gives me a 
>>>>         
>> big "Huh?".
>>     
>>>> Point 3: What if you call doIntercept() in a handleFault() 
>>>>         
>> operation?
>>     
>>>> Nothing can prevent it. If you look at the code for the
>>>> PhaseInterceptorChain, the chain is not yet ABORTED, and 
>>>>         
>> this can reek
>>     
>>>> havoc, by starting down the the rest of the chain when it should be
>>>> unwinding!
>>>>
>>>>    [There's also another problem is an interceptor removes 
>>>>         
>> itself from
>>     
>>>> the chain, or something else removes the currently executing
>>>> interceptor, but may get into that later :) ]
>>>>
>>>> Thanks for listening to my concerns.
>>>>
>>>> Cheers,
>>>> -Polar
>>>>
>>>> Dan Diephouse wrote:
>>>>         
>>>>> Hiya,
>>>>>
>>>>> On 2/14/07, Polar Humenn <phumenn@iona.com> wrote:
>>>>>           
>>>>>>             
>>>>>>> And the idea that onTermination() is called after the 
>>>>>>>               
>> chain has 
>>     
>>>> been
>>>>         
>>>>>>> invoked
>>>>>>> is just as simple to understand. Are you telling me that a 
>>>>>>>               
>>>> developer
>>>>         
>>>>>>> can't
>>>>>>> understand that a method is executed at the end of the chain?
>>>>>>>               
>>>>>> But I'm confused, onTermination() isn't called at the 
>>>>>>             
>> "end" of the
>>     
>>>>>> chain. The first call to an onTermination() method is 
>>>>>>             
>> in the "middle"
>>     
>>>> of
>>>>         
>>>>>> the chain. That's because you are still "handling" the 
>>>>>>             
>> message in
>>     
>>>>>> onTermination() calls of the interceptors. For instance, in the
>>>>>>             
>>>> outgoing
>>>>         
>>>>>> interceptor phase chain, the message isn't guaranteed 
>>>>>>             
>> to pop-out 
>>     
>>>> at the
>>>>         
>>>>>> end of the chain, it pops out at the beginning of the chain!
>>>>>>
>>>>>> Let's take the MessageSenderInterceptor. This interceptor is 
>>>>>>             
>>>> placed at
>>>>         
>>>>>> the "PREPARE_SEND" Phase. Looking at its current 
>>>>>>             
>> implementation there
>>     
>>>> is
>>>>         
>>>>>> a Chain.doIntercept() call. I will assume that it is 
>>>>>>             
>> the split where
>>     
>>>>>> handleMessage and onTermination() will now exist.
>>>>>>
>>>>>> handleMessage will call conduit.send(message);
>>>>>>           (which basically readies the message to get 
>>>>>>             
>> handled by 
>>     
>>>> other
>>>>         
>>>>>> interceptors,
>>>>>>            in the PRE_STREAM, etc. phases.)
>>>>>>
>>>>>> onTermination will call conduit.close(message);
>>>>>>           (which closes the OutputStream, i.e. flushes the final
>>>>>>             
>>>> buffers
>>>>         
>>>>>> writing data to the wire, guaranteeing that the message 
>>>>>>             
>> has been 
>>     
>>>> sent.)
>>>>         
>>>>>> So, my basic question is, and what is confusing to me, is why is
>>>>>> conduit.close(message) called in the PREPARE_SEND 
>>>>>>             
>> phase? Shouldn't it
>>     
>>>>>> logically be called in the "SEND" Phase, which is the 
>>>>>>             
>> *last* outgoing
>>     
>>>>>> phase?
>>>>>>             
>>>>> Our phases aren't very well named at the moment. Its 
>>>>>           
>> been on my todo
>>     
>>>> list
>>>>         
>>>>> for forever to clean them up.
>>>>>
>>>>> And what if that onTermination() call got an IOException on the
>>>>>           
>>>>>> OutputStream.close(). (The message really cant be said 
>>>>>>             
>> it made it out
>>     
>>>> to
>>>>         
>>>>>> the wire. None of the interceptors after the 
>>>>>>             
>> PREPARE_SEND phase will
>>     
>>>> get
>>>>         
>>>>>> notified.
>>>>>>             
>>>>> Ahhhh, I see your point now. You're right, that does get 
>>>>>           
>> sticky. I
>>     
>>>>> think you
>>>>> may be right - this may make that proposal a non 
>>>>>           
>> starter. (Sorry it
>>     
>>>>> took me
>>>>> a while.)
>>>>>
>>>>> I hope no one minds if I float two more proposals that 
>>>>>           
>> we've talked
>>     
>>>> about
>>>>         
>>>>> earlier for the sake of completeness:
>>>>>
>>>>> The first idea that we've floated before is supporting reentrant
>>>>> interceptor
>>>>> chains in a slightly different manner. The idea being 
>>>>>           
>> that instead of
>>     
>>>>> doInterceptInSubChain we would support something like this:
>>>>>
>>>>> public void handleMessage(Message m)
>>>>> startSend();
>>>>> message.getChain().doIntercept(message, endPhase)
>>>>> close();
>>>>> }
>>>>>
>>>>> This is basically just a variation on the current theme, 
>>>>>           
>> but making it
>>     
>>>>> cleaner. The main advantage of this over the current API 
>>>>>           
>> would be that
>>     
>>>>> databinding interceptors wouldn't need to call 
>>>>>           
>> chain.endSubChain().
>>     
>>>>> The second idea would be to rework writing so that it was more
>>>>> encapsulated.
>>>>> In XFire we had the idea of a Serializer. One of the final
>>>>> interceptors was
>>>>> responsible for opening the connection, doing
>>>>> serializer.write(message), and
>>>>> finally closing the connection. The idea here is that as we move
>>>>> through the
>>>>> chain we can replace/modify the serializer as we feel 
>>>>>           
>> necessary. The
>>     
>>>>> thing I
>>>>> like about this is that it completely removes the need 
>>>>>           
>> for reentrant
>>     
>>>>> interceptorchains/onTermination.  Here's an example of 
>>>>>           
>> how it might
>>     
>>>> work:
>>>>         
>>>>> 1. BareOutInterceptor sets the serializer to something 
>>>>>           
>> which writes 
>>     
>>>> the
>>>>         
>>>>> message in a doc/lit style
>>>>> 2. Outgoing SAAJ interceptor uses this serializer to 
>>>>>           
>> write the message
>>     
>>>>> to a
>>>>> SAAJ tree. The serializer is then replaced with a SAAJ serializer
>>>>> 3. Attachment out interceptor wraps the current 
>>>>>           
>> serializer with one
>>     
>>>> which
>>>>         
>>>>> starts writing the attachments, then delegates to the 
>>>>>           
>> SAAJ serializer,
>>     
>>>>> and
>>>>> then finishes writing the attachments
>>>>> 4. A MessageSenderInterceptor opens the connection, executes the 
>>>>>           
>>>> current
>>>>         
>>>>> serializer (outputting the message to the wire), and 
>>>>>           
>> then closes the
>>     
>>>>> connection.
>>>>>
>>>>> The thing that I don't like about this is that it 
>>>>>           
>> introduces a new
>>     
>>>>> concept,
>>>>> but it does become much more straightforward how to do 
>>>>>           
>> output IMO.
>>     
>>>>> And then there is the standing proposal to just completely remove
>>>>> doInterceptInSubChain() :-) Thoughts? At the moment I have a 
>>>>>           
>>>> preference
>>>>         
>>>>> toward continuing our support for reentrant interceptors 
>>>>>           
>> a la the 
>>     
>>>> first
>>>>         
>>>>> proposal that I floated, but I could be swayed either way yet.
>>>>>
>>>>> Thanks Polar,
>>>>>
>>>>> - Dan
>>>>>
>>>>>           
>>>>         
>>>       
>>     


Mime
View raw message