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 <sbery...@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: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 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 > <sbery...@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 > > clecl...@xebia.fr > > http://blog.xebia.fr > > > > On Mon, Dec 21, 2009 at 6:54 PM, Sergey Beryozkin <sbery...@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 > >>> clecl...@xebia.fr > >> > > >
