Hi Martin Ok, I read again all documentation about ExceptionHandler, and I committed some code that closes some subtasks created:
- Fix Ajax case and myfaces error handling disabled. - Add MyFacesExceptionHandlerWrapperImpl and remove ErrorPageWriter.handleThrowable try/catch blocks, this solves MYFACES-3193 - Output multiple exception stack traces - Disable MyFaces exception handling if Project Stage is not development. Before, it was enabled in stages like Production, but this is not wanted to comply with JSF 2.0 spec (Mojarra disables it too). In this way, it should be enabled in a explicit way in production. - Remove ErrorPageWriter.handleThrowable block from ResourceHandler implementation, because there is no view there and is not meaningful in such situation (serve html when text/css or text/javascript is expected doesn't sounds good). Issues still open: - MYFACES-3076 Improve error reporting and logging: report duplicate converters, validators, etc: Help is most welcome. - MYFACES-3191 Handing exception (in exception handler) from render response phase with forward/redirect is inconsistent: It seems something we need to handle at spec level. Let me think a little bit about it. - MYFACES-3198 Unify exception handling from render_response component.encodeAll: it has sense, but if there is an exception, how to warrant than the ajax response is well formed? what to do in that case? just log? how the client is notified? is some operation done on the client side?. Too many unresolved questions. At first view the safest strategy is throw the exception, so the exception handler can catch it and notify the client. From the spec point of view, ajax is an all/none operation, so it could sense but that falls outside jsf spec. - MYFACES-3199 Handling AbortProcessingException is unconsistent: We need a test case for this one. Help is most welcome. It could be good if you can check the new strategy to see if works well. regards, Leonardo Uribe 2011/6/29 Martin Koci <martin.kocicak.k...@gmail.com>: > Leonardo Uribe píše v Út 28. 06. 2011 v 16:11 -0500: >> Hi >> >> 2011/6/28 Martin Koci <martin.kocicak.k...@gmail.com>: >> > Hi, >> > >> > to make things not so easy: >> > https://issues.apache.org/jira/browse/MYFACES-3191 >> > Maybe it is related, maybe not, but now it is not possible to perform >> > navigation in RENDER_RESPONSE. >> > >> >> Yes, it it not possible to do navigation in RENDER_RESPONSE, because >> render response phase already happen, so there exists navigation but >> there is no rendering. So, a redirect is the only way to trigger a >> navigation in this case, but this means debug information must be >> saved somewhere. All that is responsible of the ExceptionHandler used. >> >> > >> > About infinite loop: similar problem (not hadled with spec) is in >> > broadcastEvents - listener can queue the same event again. Please see >> > code in javax.faces.component.UIViewRoot.broadcastEvents(FacesContext, >> > PhaseId). Situation in exception handler is similar: throwing a >> > exception again leads to queue new ExceptionQueuedEvent. >> > Maybe we should think about this problem (inifinite queuing) generally, >> > extract the code from UIViewRoot.broadcastEvents and solve it everywhere >> > in code base for all queue operations. >> > >> >> I don't see any problems with the algorithm proposed related to >> infinite loops from a theorical point of view, because after all, if >> the exception handler call UIViewRoot.broadcastEvents, it is >> responsibility of the exception handler to deal with this gracefully. >> >From the point of view of the spec and the algorithm on section 12.3, >> jsf implementation should not do anything. >> >> Anyway, I have seen some problems in our current error handling >> algorithm. Technically it comply with the spec, but note the spec >> unfortunately is very poor in this part. There is some code dealing >> with ajax exceptions, that is on ErrorPageWriter that should not be >> there, because if myfaces error handling is disabled, this code is not >> executed too, and ErrorPageWriter should be called from our >> ExceptionHandler implementation in MyFaces, instead use a try {} catch >> {} block, even if errors on ResourceHandler implementation could not >> be handled (or maybe the ExceptionHandler should do its own stuff >> here?). Additionally, our wiki page to handling errors must be >> updated, but first we need to check in deep this feature. > > Hi, > > I did something for MYFACES-3053 - Improve error reporting and logging. > If you have some time, please take a look at subtasks of that issue - > some of them are related to current error handling algorithm. > > Regards, > > Kočičák >> >> regards, >> >> Leonardo Uribe >> >> > Regards, >> > >> > >> > Kočičák >> > >> > Leonardo Uribe píše v Po 27. 06. 2011 v 18:02 -0500: >> >> Hi >> >> >> >> I investigated more about this problem and from a theorical point of >> >> view we can't call handle() like proposed. Why? in JSF we have the >> >> following phases by default: >> >> >> >> RESTORE_VIEW >> >> APPLY_REQUEST_VALUES >> >> PROCESS_VALIDATIONS >> >> UPDATE_MODEL_VALUES >> >> INVOKE_APPLICATION >> >> RENDER_RESPONSE >> >> >> >> and we have these methods on FacesContext: >> >> >> >> renderResponse(); >> >> getRenderResponse(); >> >> responseComplete(); >> >> getResponseComplete(); >> >> >> >> If a ViewExpiredException is thrown on RESTORE_VIEW, and a call to >> >> handle() occur, everything that happens inside the ExceptionHandler is >> >> its responsibility. If handleNavigation() is called inside the >> >> handler, that handler should check for a view set or >> >> getResponseComplete return true and deal with it. Now, if the handler >> >> does not do anything, the following phases: >> >> >> >> APPLY_REQUEST_VALUES >> >> PROCESS_VALIDATIONS >> >> UPDATE_MODEL_VALUES >> >> INVOKE_APPLICATION >> >> RENDER_RESPONSE >> >> >> >> By definition, these phases should check always if there is a view and >> >> if it is not, just throw ViewNotFoundException, aborting the current >> >> phase. Note it is responsibility of the phase to do the check. In that >> >> way the ExceptionHandler could have the chance to handle it. >> >> >> >> I tried do something different, but it is not possible because is >> >> inconsistent with the intention of ExceptionHandler. How can an >> >> ExceptionHandler handle exceptions thrown by itself without the risk >> >> of get stuck in a loop? >> >> >> >> regards, >> >> >> >> Leonardo Uribe >> >> >> >> 2011/6/27 Leonardo Uribe <lu4...@gmail.com>: >> >> > Hi >> >> > >> >> > 2011/6/27 Martin Koci <martin.kocicak.k...@gmail.com>: >> >> >> Hi, >> >> >> Leonardo Uribe píše v Ne 26. 06. 2011 v 21:29 -0500: >> >> >>> Hi >> >> >>> >> >> >>> I have checked all issues and this is what I think about: >> >> >>> >> >> >>> - MYFACES-3189 and MYFACES-3188 : I don't see any difference between >> >> >>> both of them. >> >> >> >> >> >> Yes, those are caused by same problem, but I reported it separately >> >> >> because MYFACES-3188 suggest only improvement in "robustness" area, >> >> >> MYFACES-3189 is about navigation algorithm. >> >> >> >> >> > >> >> > Ok, now I get it. I believe one patch will fix MYFACES-3188 and >> >> > MYFACES-3189 should be closed as invalid, because we can really do >> >> > anything on the navigation algorithm. >> >> > >> >> >>> The navigation algorithm is ok. I think with just log a >> >> >>> warning message before render response phase saying the view is null >> >> >>> is ok. I think it is reasonable to throw a custom myfaces exception >> >> >>> here because is viewRoot is null on this point, there is no way render >> >> >>> response phase could work. Maybe a ViewNotFoundException? I don't see >> >> >>> any incompatibility with the spec, but at least we should notify the >> >> >>> EG about this possible problem, because the exception class should be >> >> >>> on javax.faces.application, and the algorithm should be clear about >> >> >>> this. >> >> >>> >> >> >> >> >> >> Exception: can be this exception also handled with custom exception >> >> >> handler? For example: two views (ViewExpired.xhtml and >> >> >> NiceErrorView.xhtml) packaged in a .jar. For some reason this archive >> >> >> will no be deployed. Then if ViewExpired occurs: >> >> >> >> >> >> -- exception handler navigates to ViewExpired.xhtml -> >> >> >> ViewNotFoundException >> >> >> -- exception handler handles ViewNotFoundException and navigates to >> >> >> NiceErrorView.xhtml -> new ViewNotFoundException is throw. >> >> >> >> >> > >> >> > Good idea!. The only thing here is we need some special logic to deal >> >> > with >> >> > this. The most difficult problem is we can't keep track of the view >> >> > navigations >> >> > that cause ViewNotFoundException on the same request, but we can >> >> > set a limit of how many ViewNotFoundException can be handled before >> >> > jump to the default error page writer. This looks each time more like >> >> > something >> >> > to be included on the spec. >> >> > >> >> > regards, >> >> > >> >> > Leonardo >> >> > >> >> >> >> >> >> >> >> >> >> >> >>> - MYFACES-3105 : Checked and fixed. >> >> >>> >> >> >>> Fortunately, there is no errors on the algorithm, but anyway we can >> >> >>> expect more reports about ViewExpiredException in the future, because >> >> >>> it is a common exception that occur in typical scenarios. >> >> >>> >> >> >>> regards, >> >> >>> >> >> >>> Leonardo Uribe >> >> >>> >> >> >>> 2011/6/26 Martin Koci <martin.kocicak.k...@gmail.com>: >> >> >>> > Hi, >> >> >>> > >> >> >>> > MYFACES-3189 and MYFACES-3188 are reproducible : please see >> >> >>> > comment at >> >> >>> > MYFACES-3189. The mojara example works ok, thats not the problem. >> >> >>> > The >> >> >>> > problem is when you makes a typo in name of view for >> >> >>> > handleNavigation >> >> >>> > method. >> >> >>> > >> >> >>> > >> >> >>> > >> >> >>> > MYFACES-3105 seems fixed in current 2.1.2-SNAPSHOT >> >> >>> > >> >> >>> > Regards, >> >> >>> > >> >> >>> > Kočičák >> >> >>> > >> >> >>> > Leonardo Uribe píše v So 25. 06. 2011 v 11:49 -0500: >> >> >>> >> Hi >> >> >>> >> >> >> >>> >> I have tried to reproduce them without success. I know the >> >> >>> >> navigation >> >> >>> >> code and everything seems to be correct. Do you have a test case >> >> >>> >> for >> >> >>> >> this one? I tried the bundled sample from mojarra and it works. >> >> >>> >> >> >> >>> >> regards, >> >> >>> >> >> >> >>> >> Leonardo Uribe >> >> >>> >> >> >> >>> >> 2011/6/25 Martin Koci <martin.kocicak.k...@gmail.com> >> >> >>> >> Hi, >> >> >>> >> >> >> >>> >> please take a look at: >> >> >>> >> >> >> >>> >> >> >> >>> >> https://issues.apache.org/jira/browse/MYFACES-3189 >> >> >>> >> https://issues.apache.org/jira/browse/MYFACES-3188 >> >> >>> >> https://issues.apache.org/jira/browse/MYFACES-3105 >> >> >>> >> >> >> >>> >> I'm not very familiar with navigation implementation - I >> >> >>> >> cannot provide >> >> >>> >> meaningful patches here. >> >> >>> >> >> >> >>> >> >> >> >>> >> Thanks, >> >> >>> >> >> >> >>> >> >> >> >>> >> Kočičák >> >> >>> >> >> >> >>> >> >> >> >>> > >> >> >>> > >> >> >>> > >> >> >>> >> >> >> >> >> >> >> >> >> >> >> > >> >> >> > >> > >> > >> > > >
