Hi Jervis,
Comments in line.
Liu, Jervis wrote:
-----Original Message-----
From: Polar Humenn [mailto:[EMAIL PROTECTED]
Sent: 2007?2?13? 3:33
To: [email protected]
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 <[EMAIL PROTECTED]> 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
