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, 18 Jan 2010 00:06:01 GMT
   Hello Sergey,

   I added thread local variables cleanup in
JAXRSInInterceptor.handleFault() as you suggested on both trunk and
2.2-fixes. It will be available in 2.2.6. Change is tracked in
"[CXF-2622] ThreadLocal variables may not be cleared in case of
exception" (1).

   I will continue to work on the refactoring to get JAXRS monitoring
with exception counting for 2.3 and maybe 2.2.7 :-) .

   Cyrille

(1) https://issues.apache.org/jira/browse/CXF-2622

On Mon, Jan 11, 2010 at 12:50 PM, Sergey Beryozkin
<sberyozk@progress.com> wrote:
>
> 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
View raw message