Re: Questions regarding JAX-RS exception handling
Hi Cyrille
Thanks for fixing it, a very important fix indeed and sorry for a delay in
replying to this thread.
Please see some comments inline, I'll do some snips along the way...
thanks, Sergey
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 :-) .
S.B : please do, thanks. I'm just thinking may be we should target 2.3 only due to the minor issue to do with the fact that some
JAXRS users might need to register a custom FaultLogger in order to avoid some excessive logging ? We will document it in the
migration guide from 2.2.x to 2.3 ? Some more comments are below...
Cyrille
(1) https://issues.apache.org/jira/browse/CXF-2622
>> 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, , MessageBodyWriter, ResponseHandler and ExceptionMapper ?
S.B : yes
>> CLC: if so, almost each thread local is easy to cleanup in a finally block
because its scope is limited to one single method :
>> CLC: the only challenge I see is limited : the 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.
S.B : thanks for eliminating this potential source of leaks with your latest fix
>> 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.
S.B : can you elaborate a bit more please ? Does handleFault meet this requirement or would you like to propose some enhancements
to the way PhaseInterceptorChain operates ?
>> 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.
S.B : sounds interesting but would we need to update PhaseInterceptorChain to rewind the chain in the case of Propagated exception
? And perhaps doing some more coding around it ? Even if it is meant to be propagated, it should still go through the fault
chains, say for a management feature to work ? What do you think about updating AbstractFaultChainInitiatorObserver, for it to
check a propagated exception property on a message/exchange and if it is set then not to log ?
* XMLFaultOutInterceptor and StaxOutInterceptor are no longer invoked
in the faultOut chain, see JAXRSFaultOutInterceptor
>> 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.
S.B : ok. I guess I'm confused by the fact you said XMLFaultOutInterceptor would not be invoked ? But XMLFaultOutInterceptor is
the one which will render the exception in xml if no exception mapper has been found ?
>> 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.
S.B : this is probably ok, no problems here...
>> CLC: Do you see problems in handling the ExceptionMapper step as the first
step of the fault out chain ?
S.B : not really, should work ok...
>> CLC: The resource cleanup concern in the continuations scenario would be
fixed by the usage of finally blocks described above.
S.B : in JAXRSInInterceptor ?
>> CLC: As I said above, I would see benefits in evolving the PhaseInterceptorChain to handle "propagated exceptions". Adding
>> methods t
Re: Questions regarding JAX-RS exception handling
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
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:clecl...@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
> clecl...@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, , 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 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
>
> &g
RE: Questions regarding JAX-RS exception handling
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:clecl...@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
clecl...@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, , 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 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 Ph
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
clecl...@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, , 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 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 dea
RE: Questions regarding JAX-RS exception handling
Hi Cyrille Thanks for looking into this issue, it all seems to be going really well, at some stage seeing the patch would help to see the whole picture better. Please see some comments prefixed with S.B inline -Original Message- From: Cyrille Le Clerc [mailto:clecl...@apache.org] Sent: Mon 1/4/2010 3:01 PM To: dev Subject: Re: Questions regarding JAX-RS exception handling 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, > 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 ? * 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. 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 * 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 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. > 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. >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 unti
Re: Questions regarding JAX-RS exception handling
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 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
Re: Questions regarding JAX-RS exception handling
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 clecl...@xebia.fr http://blog.xebia.fr On Mon, Dec 21, 2009 at 6:54 PM, Sergey Beryozkin 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'
Re: Questions regarding JAX-RS exception handling
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 :-) 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 * the JAXRSCheckFaultInterceptor handles faults and the ResponseExceptionMapper processing 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 :-) 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 clecl...@xebia.fr http://blog.xebia.fr On Mon, Dec 21, 2009 at 6:54 PM, Sergey Beryozkin 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 forwardSome of its code > would definitely
Re: Questions regarding JAX-RS exception handling
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 :-) 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 * the JAXRSCheckFaultInterceptor handles faults and the ResponseExceptionMapper processing 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 :-) 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 clecl...@xebia.fr http://blog.xebia.fr On Mon, Dec 21, 2009 at 6:54 PM, Sergey Beryozkin 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 forwardSome of its code > would definitely
Re: Questions regarding JAX-RS exception handling
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 forwardSome 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 clecl...@xebia.fr
