cxf-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Cyrille Le Clerc <clecl...@apache.org>
Subject Re: Questions regarding JAX-RS exception handling
Date Mon, 04 Jan 2010 20:01:54 GMT
   Hello,

   Here is an updated version of the refactoring of the server side
handling of exceptions. It passes most of the systests, there is a
message format issue if no writer is found, all there other tests seem
to pass. Here are the details :

* 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,

* 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

* 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 ? Why missing writers error message is currently rendered as
plain text and not XML as other faults ?

* 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.

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.

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
:-)

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/mixed (inline, None, 0 bytes)
View raw message