On 13/06/17 11:05, Violeta Georgieva wrote: > 2017-06-13 13:04 GMT+03:00 Violeta Georgieva <violet...@apache.org>: >> >> Hi, >> >> >> 2017-06-12 21:43 GMT+03:00 Mark Thomas <ma...@apache.org>: >>> >>> On 09/06/17 16:26, Violeta Georgieva wrote: >>>> 2017-06-09 17:25 GMT+03:00 Mark Thomas <ma...@apache.org>: >>> >>> <snip/> >>> >>>>> I've spent some time working through the various possible > combinations >>>>> of events and have concluded it is impossible to completely fix this >>>>> without imposing additional requirements on applications that the >>>>> specification doesn't mention. >>>>> >>>>> However, I believe that we can do better than the current >>>>> implementation. What I have on mind would: >>>>> >>>>> - always trigger AsyncListener.onError() for all listeners >>>>> - generally, process the complete() dispatch() call from the >>>>> AsyncListener rather than any from the non-container thread >>>>> - generally, throw an ISE if complete() or dispatch() is called >>>>> from the non-container thread after that thread experiences an I/O >>>>> error >>>>> - leave a small timing window where it was possible that the > complete() >>>>> or dispatch() from the non-container thread would be used rather > than >>>>> from the AsyncListener. In that case the AsyncListener would see > the >>>>> ISE but any remaining AsyncListener instances would still be called >>>>> >>>>> I don't see a way of doing better than this without spec changes / >>>>> clarifications. >>>>> >>>>> WDYT? >>>> >>>> +1 >>>> I'm able to test the new behavior with my real web app. >>> >>> Excellent. I've committed my proposed fix. The async unit tests pass >>> which is generally a good sign. If this works better with your real web >>> application then we can look to back-port this. >> >> I'm seeing now the following exceptions:
In the same run or different runs? > I back ported the fix to my local 8.5 branch in order to be able to test it > ... > >> >> java.lang.IllegalStateException: Calling [asyncComplete()] is not valid > for a request with Async state [MUST_DISPATCH] It appears the application called dispatch() from a non-container thread once the error state had been entered. We could block that but there is a risk it will break valid use cases. Fundamentally, I don't believe the app should be doing this. >> at > org.apache.coyote.AsyncStateMachine.doComplete(AsyncStateMachine.java:317) > ~[tomcat-embed-core.jar!/:8.5.16-dev] <snip/> >> ================== >> >> java.lang.NullPointerException: null Appears to be the same cause as above, but triggered at a different point. >> at > org.apache.catalina.core.AsyncContextImpl.setErrorState(AsyncContextImpl.java:411) > ~[tomcat-embed-core.jar!/:8.5.16-dev] <snip/> This is consistent with what I'd expect. In both of the above cases any AsyncListener.onError() methods should execute but if the app continues to do things on the non-container thread then there possibility of exceptions remains. Mark --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
