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