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 Wed, 14 Feb 2007 19:03:25 GMT
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