cxf-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sergey Beryozkin <sbery...@progress.com>
Subject RE: Questions regarding JAX-RS exception handling
Date Mon, 11 Jan 2010 11:50:04 GMT
Hi Cyrille

thanks for your comments. No problems with the delay, you're leading this thread really well
and I'm learning few bits myself too...I'll have to sign off shortly so will comment later
on, I will have some time to think about what you suggested...
I think it all looks/sounds quite good. May be we also need to modify JAXRSInInterceptor to
add a handleFault method and ensure no leak occurs even if the fault is thrown by some custom
CXF interceptor sitting between JAXRSInInterceptor and JAXRSInvoker, this fix can be added
independently, may be before 2.2.6...
I'll add more comments later on.

thanks, Sergey

-----Original Message-----
From: Cyrille Le Clerc [mailto:cleclerc@xebia.fr]
Sent: Sat 1/9/2010 7:16 PM
To: dev@cxf.apache.org
Subject: Re: Questions regarding JAX-RS exception handling
 
Hello Sergey,

Thank you to have taken the time to read my long email. I may not have
been clear enough on the behavior of the exception mapper handling I
suggest in my previous email, I tried to clarify it.

I add comments prefixed by "CLC:" in the text ; I hope I answered to
all your points.

I can propose a new version of the patch with the new thread locals
cleanup and an improved exception propagation to the servlet
container.

Cyrille

PS : sorry for the delay of my answers but it takes me time to better
understand CXF internals and JAX-RS specs :-)
--
Cyrille Le Clerc
cleclerc@xebia.fr
http://blog.xebia.fr

==========================

* Message serialization is mutualized in a AbstractJAXRSOutInterceptor
from which both JAXRSOutInterceptor and JAXRSFaultOutInterceptor
inherit, there is no longer the weird concept of a third interceptor
chain,

* Thread locals and reserved resources release are moved in a
JAXRSResourceCleanerOutInterceptor that is added to both the out and
faultOut interceptors chain,

> S.B. As you note below, there is a minor possibility of a leak if a given chain is aborted
earlier on. We can of course warn users to make sure they do the cleanup if they try to abort
the chain in their custom out/fault interceptors, but I'd really like to make sure no leak
occurs, no matter what users do. So shall we move the cleanup code into AbstractJAXRSOutInterceptor
and update JAXRSOutInterceptor and JAXRSFaultOutInterceptor to clean up in their finally blocks
?

>> CLC: also convinced that risking to not cleanup thread locals is worrying,
>> CLC: can you confirm my understanding that thread locals are related to @Context
and @Resource fields/setters and only apply to RequestHandler, MessageBodyReader, <serviceObject/Resource>,
MessageBodyWriter, ResponseHandler and ExceptionMapper ?
>> CLC: if so, almost each thread local is easy to cleanup in a finally block because
its scope is limited to one single method : JAXRSInInterceptor.handleMessage(), JAXRSInvoker.invoke(),
JAXRSOutInterceptor.handleMessage() and JAXRSFaultOutInterceptor.handleMessage()
>> CLC: the only challenge I see is limited : the <serviceObject/Resource> is
injected thread locals in the JAXRSInInterceptors and the cleanup is done in the JAXRSInvoker.
I didn't yet figure how the thread locals are cleared if an exception occurs between the two.

>> CLC: as a general evolution, I would see great benefits in adding a "finally" semantic
in the interceptors. I already saw use cases with implementing "circuit breaker patterns"
or "invocation concurrency limitations" (with semaphores) ; we do such things on my project
with "try {} finally {}" blocks in the service implementation because we fear leaks due to
aborted chains executions.

* JAXRSInvoker and JAXRSInInterceptor lose all their exception
handling and resource cleanup (thread local, etc) logic, they just
rethrow to let the PhaseInterceptorChain invoke the adhoc interceptors

> S.B this is related to the above comment. I guess I'm slightly nervous about postponing
the cleanup until later :-).

* JAXRSOutInterceptor gives most of its business logic to the
AbstractJAXRSOutInterceptor  (all the message serialization),

* JAXRSFaultOutInterceptor handles all the exception handling logic
(ExceptionMapper) :
 ** TODO : why the default fault is render in XML ? why not plain
text ?

>S.B : this is what XMLFaultOutInterceptor does by default, and in fact, I know that some
users would like just this, that is an xml-formatted error description, if no ExceptionMappper
has been found and if the fault propagation to the container (in the form of ServletException)
has been disabled.

>> CLC: Understood for the XMLFaultOutInterceptor.
>> CLC: Regarding the exception propagation to the servlet container, would it make
sense to add a dedicated mechanism in the PhaseInterceptorChain, I imagine it similar to the
invocation suspension with the SuspendedInvocationException. A PropagatedException would hold
the underlying exception (ServletException, IOException or RuntimeExcetpion) and it would
bubble until the ServletController.invoke() where the actual exception would be thrown. It
currently goes throught the step "..AbstractFaultChainInitiatorObserver - Error occurred during
error handling, give up!" that seems to be dedicated to abnormal behaviors.

Why missing writers error message is currently rendered as
plain text and not XML as other faults ?

> S.B : this is handled by a default WebApplicationExceptionMapper, thus the fault does
not reach the fault chain
>> CLC: understood.

* XMLFaultOutInterceptor and StaxOutInterceptor are no longer invoked
in the  faultOut chain, see JAXRSFaultOutInterceptor

* JAXRSResourceCleanerOutInterceptor is associated both with the out
and the faultOut interceptor chains. Clean the thread locals and the
the reserved resource (resourceProvider.releaseInstance ).
 ** TODO : should we deal with ContextClassLoader ?
 ** TODO : should we cleanup both on handleMessage and handleFault ?
 ** there is a problem if someone wants to abort the fault chain (see
testcase 1), it would bypass the cleanup.

> S.B : yes, it is a bit worrying
>> CLC : same feeling.

Regarding your question about the noisy logging of application
exception by the PhaseInterceptorChain, I feel that the concept of
checked application fault should do the job ; I didn't verify.

> S.B. It seems like the logging will occur no matter how we set the fault code, though
the levels will differ. There is a pending patch for PhaseInterceptorChain be able to check
for custom FaultLoggers which should do the job.
>> CLC : the FaultLogger seem to be the good path.

> S.B. I'm wondering, should we try to step back a bit and consider more seriously your
initial idea about explicitly invoking a fault chain if an exception mapper has been found
?
The only 2 problems that we need to address are these : if an
(application) fault has been mapped to a Response by ExceptionMapper
then custom in/out interceptors which are registered earlier/later in
the chain will not have their handleFault methods called (1) and the
fault chain will be bypassed (2). I'm not sure if users use custom out
interceptors after JAXRSOutInterceptor has been invoked so the former
problem is less critical but the latter problem prevents JAXRS users
from *fully* utilizing some core CXF features, specifically,
exceptions are not counted/checked properly by the management feature,
but only when they have been converted into JAXRS Response by mappers.

>> CLC: I may not have been clear enough. If an exception is thrown, I propose to let
the PhaseInterceptorChain handle it normally, that is to say unwind the in interceptors chain
(calling handleFault()) and to trigger the fault out chain. If the exception is mapped to
a response, the fault chain will render this response, otherwise, the fault chain will render
the exception in xml or propagate it to the servlet container.
>> CLC: I see one thing you may dislike : custom out interceptors must also be registered
as out fault interceptors to be called even if exceptions are thrown ; this is similar to
the soap chains behavior.
>> CLC: Do you see problems in handling the ExceptionMapper step as the first step of
the fault out chain ?


>S.B The solution which which we've discussed so far seems the best one technically but
there're few bits I'm not feeling comfortable about...JAXRS users will need to do extra work
which they haven't had to do before if they'd like to minimize the logging noise and postponing
the cleanup until the later stages seems a bit brittle. I forgot to mention that JAXRS users
can avail of the CXF Continuations API so the runtime SuspendedInvocations will occur as a
result and no fault chains will be invoked to handle them given that it is not a real fault,
at some later time some other thread will get back and pass through the JAXRSInInterceptor
again so this is where we can have a leak after the given invocation has been suspended if
we do a cleanup in the fault chain, possibly a number of times - may be we can fix it by updating
JAXRSInInterceptor/JAXRSInvoker only if SuspendedInvocation has occured.

>> CLC: the logging concern should be solved by the FaultLogger that has been committed
in the trunk.
>> CLC: The resource cleanup concern in the continuations scenario would be fixed by
the usage of finally blocks described above.


What you've done so far seems very precise and quite perfect but as
you see yourself the refactoring is becoming a bit complicated :-). In
the end of the day, writing a simple FaultLogger which will block the
extra logging or ensuring the cleanup always occurs is not a big deal,
but before we commit to it I'd like us to explore an alternative
solution.

> S.B So the possible alternative approach is to ensure an in/out fault chain is called
explicitly even after we have mapped an exception to JAXRS Response with a custom ExceptionMapper.
I'm not sure how to do it yet, may be we can add some method to PhaseInterceptorChain, say,
getCurrentFaultChain.
Or may we can add a property like
"org.apache.cxf.exception.convertedToResponse" and rethrow the fault
and update XMLOutFaultInterceptor to check this property and not to
write out if it 's been set...

>> CLC: As I said above, I would see benefits in evolving the PhaseInterceptorChain
to handle "propagated exceptions". Adding methods to interact with the fault chain could be
tricky as, in case of fault, the PhaseInterceptorChain simply invokes a fault MessageObserver
that just happens to hold a fault interceptor chain but the MessageObserver interface doesn't
hold such a meaning. More over, the fault chain is created on demand.

What do you think ?

thanks, Sergey



Regarding your question about the client, I didn't touch the WebClient
yet, it is on my todo list there should not be problems with it.

I would prefer to focus on the server side right now and postpone the
client side refactoring as the server side  work is already pretty big
:-)

> S.B agreed :-)

Please tell me if it makes sense to continue to work on this,

Cyrille

(1) see org.apache.cxf.systest.jaxrs.CustomOutFaultInterceptor in jaxrs systest


On Tue, Dec 29, 2009 at 12:48 PM, Sergey Beryozkin
<sberyozk@progress.com> wrote:
>
> Hi Cyrille
>
> Thanks for posting this proposal/analysis, please see some comments
> prefixed with S.B. below...
>
> cheers, Sergey
>
> Hello all,
>
> Here is a proposal of refactoring of both the JAXRS client-side and
> server-side, these refactoring could be separated one from the other.
>
> Please, let me know if it worth continuing this work.
>
> SERVER SIDE
> ============
>
> Move the ExceptionMapper handling from the JAXRSInvoker to a new
> JAXRSFaultOutInterceptor.
>
> Description : If an exception is associated with a Response via an
> ExceptionMapper, the fault interceptors chain is aborted and a new
> chain is triggered to render the Response.
>
> Pros : consistency between the JAXRS and JAXWS interceptor chains, for
> example, the ResponseTimeFeature can now count exceptions mapped to
> responses.
>
> Cons : a third interceptors chain is introduced for exceptions that
> are mapped to Response. It is a bit weird :-)
>
> S.B :
> It looks like the right approach going forward from a technical perspective. Note that
at the moment JAXRSInvoker, in JAXRSInInterceptor and out JAXRSOutInterceptor are all trying
to map exceptions to Responses given that the exceptions may be thrown from the application
code (JAXRSInvoker mapping), from JAXRS message body readers or custom in filters (JAXRSInInterceptor
mapping) or from JAXRS message body writers (JAXRSOutInterceptor mapping).
>
> Perhaps, the ExceptionMapper handling can indeed be moved from the JAXRSInvoker and JAXRSInInterceptor
to the fault interceptor, but this fault interceptor should basically reuse the JAXRSOutInterceptor
if/after it has managed to map a fault to the Response given that a Response created by a
given ExceptionMapper still has to go through the chain of custom out filters and JAXRS writers.
But there are few more things to consider :
>
> - JAXRSInInterceptor/JAXRInvoker in its final block clears thread local proxies (representing
UriInfo/etc) which may've been injected into custom providers, including exception mappers,
so these proxies will need to be available for these mappers and for JAXRS writers/outFilters
(in case they need to handle the exception mapper Responses) if they will be invoked from
the fault interceptors. So the fault interceptor will need to take care of clearing all the
proxies injected into various providers in the end.
>
> - At the moment PhaseInterceptorChain will log all the faults which are coming through
it. This is something which users may not always want. For example, a JAXRS application code
might've logged the fact that a certain resource is not available and throw BookNotFoundException
and expect a custom mapper to quietly turn it into 404. At the moment it will work as expected
but if we move the mapping code from JAXRSInvoker and JAXRSInInterceptor to the fault one
then more runtime logging will get done. I think one of CXF users was thinking of customizing
PhaseInterceptorChain so that 'quiet' loggers can be plugged in but nothing has been done
yet there AFAIK.
>
> So it all should work quite well, but we need to do a bit more analysis of what it would
take to complete this refactoring on the server side...
>
> CLIENT SIDE
> ===========
>
> Extract the marshalling and exception processing logic from the jaxrs
> client to interceptors ; I only worked on the ClientProxyImpl, the
> work on the WebClient is still to do.
>
> Description :
> * the JAXRSResponseInterceptor builds the Response object (currently
> with the inputstream object). Remaining : complete the Response
> processing with the unmarshalling of the inputstream
>
>> S.B. I guess this one should probably be handling both proxy and WebClient responses
?
>
> * the JAXRSCheckFaultInterceptor handles faults and the
> ResponseExceptionMapper processing
>
>> S.B : one thing to be aware of here is that if either a code using proxy or WebClient
explicitly expects a JAXRS Response object then it should get Response...
>
> Pros : consistency betwen JAXRS and JAXWS interceptor chains, for
> example, the ResponseTimeFeature can now count exceptions mapped to
> responses.
>
> Cons : I didn't find any :-)
>
> S.B : sounds good :-)
>
> Todo : complete the cleanup of the client
>
> Note : the ClientFaultConverter should NOT be declared as an "In Fault
> Interceptor" for JAXRS endpoints (specially important for the client)
> as the ClientFaultConverter tries to unmarshall a SOAP XML exception.
>
> Cyrille
>
> --
> Cyrille Le Clerc
> cleclerc@xebia.fr
> http://blog.xebia.fr
>
> On Mon, Dec 21, 2009 at 6:54 PM, Sergey Beryozkin <sberyozk@progress.com> wrote:
>>
>> Hi Cyrille
>>
>> Please see comments inline
>>
>>>
>>> Dear all,
>>>
>>> I am looking at the consistency of exception handling among JAX-WS
>>> and JAX-RS. My primary goal is to ensure cxf management metrics (JMX)
>>> are consistent.
>>>
>>> Here are few questions :
>>>
>>> SERVER SIDE JAXRS EXCEPTION MAPPER
>>> ====================================
>>>
>>> If an ExceptionMapper handles the exception :
>>>
>>> 1) The JAXRSInvoker returns a Response instead of throwing an Exception
>>
>> Yes, this is for JAXRS message body writers be able to handle whatever Response entity
a given mapper might've set up.
>>
>>>
>>> 2) Thus PhaseInterceptorChain does NOT see that an exception occurred
>>> during the invocation
>>
>> Yes
>>
>>>
>>> 3) Thus the "Out Interceptors" are not replaced by the "Out Fault
>>> Interceptors" and these "Out Interceptors" are called on
>>> #handleMessage() with the outMessage (ie the response created by the
>>> ExceptionMapper) instead of being called on #handleFaultMessage() with
>>> the inMessage when information like the FaultMode is still holded by
>>> the inMessage
>>
>> Yes
>>
>>>
>>> 4) Interceptors like the ResponseTimeMessageOutInterceptor who rely on
>>> the faultMode attribute located on the Message that is being passed to
>>> handleMessage/handleFault are lost, they don't find the information
>>> they look for
>>
>> I see...
>>
>>>
>>> Questions :
>>> * Wouldn't it make sense to call the "Out Fault Interceptors" if a
>>> JAX-RS exception is mapped to a custom response ?
>>
>> Now that you suggested it, perhaps, one alternative in mapping exceptions to exception
mappers would be to
>> register JAX-RS specific fault interceptors which will do the mapping, instead of
doing it in the JAXRSInInterceptor or JAXRSInvoker...
>> So other registered fault interceptors will get their chance as well...
>>
>> What complicates things a bit is that JAXRS users can have ResponseHandler filteres
registered which can override the ExceptionMapper responses...
>>
>>> * which message should be given to the handleFaultMessage() ? The
>>> inMessage that caused the exception and that holds the exception as an
>>> attribute (it would be consistent with JAX-WS) or the outMessage as
>>> currently done ?
>>
>> Perhaps we should consider introducing JAXRS fault interceptors ? They will do Exception
Mapping and abort the chain if the mapping has been found ? I'm not yet sure how feasible
and/or sensitive such a change might be, but may be it will be the right step forward
>>
>>>
>>> CLIENT SIDE JAXRS EXCEPTION HANDLING
>>> =============================================
>>>
>>> ClientProxyImpl handles exceptions after calling the interceptors
>>> when, with JAX-WS, exception handling (CheckFaultInterceptor) is
>>> performed in the POST_PROTOCOL phase.
>>>
>>> Due to this, the "In Interceptors Chain" is called instead of the "In
>>> Fault Interceptors Chain" and interceptors like
>>> ResponseTimeMessageInInterceptor don't see the Response as an
>>> exception.
>>>
>>> Question :
>>> Can we imagine to refactor jaxrs client side exception handling as a
>>> post protocol interceptor ?
>>
>> The client side needs some refactoring going forward....Some of its code would definitely
need to be moved to some isolated interceptors. However, please see JAXRSSoapBookTest, Eamonn
did quite a few tests with faulty features/interceptors/server faults...
>>
>>>
>>>
>>> I hope this email was not too long ; it took me few hours to check all
>>> these use cases and figure out how it worked :-)
>>
>> No problems :-), please type as long a message as you'd like to :-), thanks for starting
this thread
>>
>> cheers, Sergey
>>
>>>
>>> Cyrille
>>> --
>>> Cyrille Le Clerc
>>> cleclerc@xebia.fr
>>
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message