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