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, * 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 * 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 ? Why missing writers error message is currently rendered as plain text and not XML as other faults ? * 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. 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. 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 :-) 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 >> >
