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
