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, 18 Jan 2010 10:32:07 GMT
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, <serviceObject/Resource>, 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 <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.
>

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

> S.B : sounds convincing...

>
> thanks, Sergey
>
>


Mime
View raw message