Hi, 2017-06-13 15:44 GMT+03:00 Mark Thomas <ma...@apache.org>: > > 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.
Ok I understand. > > >> 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. > What about at least not throw NPE? > >> 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. > Ok I understand. Can we back port the change to Tomcat 8.5 so that it is guaranteed that the AsyncListeners will be invoked on error. Thanks, Violeta > Mark > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > For additional commands, e-mail: dev-h...@tomcat.apache.org >
