Re: [servlet-dev] Async sendError with error pages?

[Mark Thomas <markt@xxxxxxxxxx>]

On 09/07/2019 14:15, Greg Wilkins wrote:
> Mark, Stuart, et al,

Greg,

There are enough edge cases and moving parts here that I want to run
some tests to confirm exactly what happens in some cases. I'll update
this thread when I have some concrete information.

Another factor to bring in to this is the unhandled exception.

Mark


> 
> I think that jetty will change to follow the implementations of Tomcat
> and Undertow.   Which if I understand correctly is as follows for an
> non-async caller:
> 
>   * Thread A Normal REQUEST dispatch
>   * Thread A calls sendError (state changed, output closed, but response
>     not committed)
>   * Thread A returns from REQUEST dispatch
>   * Thread A output reopened, ERROR dispatch.
>   * Thread A generates error page (perhaps calling startAsync and an
>     extended lifecycle).
> 
> But we'd also like to handle async callers of sendError.   Do your
> containers handling it like this?:
> 
>   * Thread A Normal REQUEST dispatch
>   * Thread A calls startAsync
>   * Thread B calls sendError  (state changed, output closed, but
>     response not committed)
>   * Winner of race between Thread A returning from REQUEST dispatch and
>     Thread B calling complete() does noop
>   * Loser of race reopens output and does ERROR dispatch
>   * Thread A/B generates page (perhaps calling startAsync and an
>     extended lifecycle).
>   * Thread A/B calls onComplete when ERROR dispatch returns or second
>     async lifecycle completes.
> 
> I think that works for an async thread calling sendError then complete,
> but there are some edge cases that need to be considered.  What if it
> calls sendError and then AsyncContext.dispatch? What if it calls
> sendError and then times out?
> 
> I think that sendError followed by AsyncContext.dispatch should be
> handled with an ISE, just like trying to use a normal dispatcher would
> if sendError had been called.
> 
> I'm not sure at all what to do on the timeout.   If there is no
> onTimeout listener, then we'd do a ERROR dispatch for the timeout, so
> perhaps we do the same:
> 
>   * we call any onTimeout listeners, if they call dispatch, then it is
>     ISE. If they call complete then we have our complete call and
>     proceed as above.
>   * If there are no onTimeout listeners or if they don't call complete,
>     then we just do the ERROR dispatch for the sendError rather than for
>     the timeout?
> 
> 
> Thoughts?
> 
> 
> On Mon, 8 Jul 2019 at 04:42, Stuart Douglas <sdouglas@xxxxxxxxxx
> <mailto:sdouglas@xxxxxxxxxx>> wrote:
> 
> 
> 
>     On Sat, 6 Jul 2019 at 02:33, Mark Thomas <markt@xxxxxxxxxx
>     <mailto:markt@xxxxxxxxxx>> wrote:
> 
>         On 05/07/2019 10:28, Greg Wilkins wrote:
>         >
>         > All, as much fun as it is talking about name changes, I'd like
>         to have a
>         > discussion about the actual semantics of the API :)
> 
>         What? And make some real progress? It will never catch on.
> 
>         > We (jetty-project) are currently reviewing how we handle
>         sendError when
>         > invoked asynchronously with an error page mapping.    I'm not
>         sure we
>         > really have the right semantic in all situations for the
>         combinations of
>         > async caller and async error page.
> 
>         Are there use cases for async error pages? My expectation is
>         that error
>         pages would either be static or would maybe dynamically add some
>         error
>         information from the request.
> 
>         I'm not saying we shouldn't support async error pages but it
>         does seem
>         ... odd.
> 
>         Generally, Tomcat differs in that when sendError() is called it
>         sets an
>         error flag but waits until control has returned to the container to
>         dispatch to the appropriate error page. The expectation is that
>         the app
>         returns control immediately after calling sendError() but if it
>         doesn't
>         Tomcat just ignores anything the app tries to write after
>         sendError()
> 
>         More specific commentary in-line for each case.
> 
>         > *Non async caller with non async error page. *
>         > This is the normal case and the error page is invoked with and
>         ERROR
>         > dispatch from within the scope of the sendError call.  After
>         returning,
>         > the response is committed and we close the output, so no more
>         can be
>         > written.  The javadoc on says this should be considered:
>         >
>         >     * After using this method, the response should be considered
>         >     * to be committed and should not be written to.
>         >
>         > But jetty does enforce it (and I think the TCK tests for this).
> 
>         Tomcat enforces this and silently swallows any further attempts
>         to write
>         to the response. This restriction is lifted once the error handling
>         takes over so the error page can be written to the response.
> 
> 
>     Undertow is the same.
>      
> 
>         > *Non async caller with async error page.*
>         > Here sendError does and ERROR dispatch to the error page,
>         which then
>         > calls startAsync and returns.  Now sendError can't commit nor
>         close the
>         > response, because it is in a race with async threads that may
>         be writing
>         > the response.   So we use an isAsyncStarted check to avoid
>         > committing/closing... so I guess that explains why the javadoc
>         only says
>         > "considered to be ..."?
> 
>         Tomcat avoids this race because of when it performs the dispatch.
> 
> 
>     Undertow also does the same, when sendError is called the currently
>     running request return to the container before actually doing the
>     error dispatch. 
>      
> 
> 
>         > *Async caller with non-async error page.*
>         > A normally dispatched request has called startAsync and then
>         an async
>         > thread calls sendError.  Current releases of jetty do an ERROR
>         dispatch
>         > to the error page from within the scope of sendError, but we
>         now realise
>         > that is not correct because the original dispatched thread may
>         not yet
>         > have exited and we will end up with 1 threads inside the servlet
>         > container (and filters etc.) for the same request at the same
>         time.   
>         >
>         > So we are considering making the error page dispatch an
>         > AsyncContext.dispatch.   This solves the concurrency problem,
>         but gives
>         > us other problems.  Should the dispatch type now be ASYNC or
>         ERROR?
>         > There is no ASYNC_ERROR!     Should the calling thread call
>         > AsyncContext.complete()?  Typically it will because that is
>         the contract
>         > of async and the caller has no idea if an async dispatch is
>         done or not
>         > inside the sendError call?  So assuming they do call
>         complete(), should
>         > we just treat it like a noop rather than ISE ?
> 
>         I'd lean towards using ERROR.
> 
> 
>     +1
>      
> 
>         I think the calling thread should call complete().
> 
>         What is the dispatch target's justification for calling
>         complete()? That
>         seems wrong to me so an ISE looks reasonable.
> 
> 
>     I don't know if it is really in line with the spec but we just
>     implicitly call complete() here when the error dispatch is done. 
>      
> 
> 
>         > *Async caller with Async error page.
>         > *
>         > A normally dispatched request has called startAsync and then
>         an async
>         > thread calls sendError, which does an async (or error?)
>         dispatch to the
>         > error page that then calls startAsync again.
> 
>         That isn't legal is it? Tomcat would throw an ISE in that situation.
> 
> 
>     We would also throw an ISE.
>      
> 
> 
>         >  We now how two threads
>         > that are acting on the same async request.  Do we wait for
>         both to call
>         > complete() before being complete? Surely we have to prevent a
>         complete
>         > call from the sendError thread from completing the async cycle
>         of the
>         > error page startAsync?  What if one of them calls
>         > AsyncContext.dispatch?   At this point we are thinking that once a
>         > request goes async, then perhaps we should never dispatch to
>         an error
>         > page from sendError and instead just generate our own simple
>         error page?
>         >
>         > I'd love to know what the other containers do in these situations?
>         >
>         > I think we obviously need to at least be more specific in our
>         javadoc,
>         > but I'm thinking that perhaps we might need to change
>         behaviour as well
>         > to avoid async on async error pages?
> 
>         I think being more specific would be a good thing.
> 
>         Would stating that targets of ERROR dispatches must be synchronous a
>         possible way forward?
> 
> 
>     I don't really like this. My gut feeling is that that sendError
>     should work like javax.servlet.AsyncContext#dispatch, and basically
>     finish the current async cycle. If the error page wants to do its
>     own async processing then it can just call startAsync again.
> 
>     Stuart
>      
> 
> 
>         Mark
>         _______________________________________________
>         servlet-dev mailing list
>         servlet-dev@xxxxxxxxxxx <mailto:servlet-dev@xxxxxxxxxxx>
>         To change your delivery options, retrieve your password, or
>         unsubscribe from this list, visit
>         https://www.eclipse.org/mailman/listinfo/servlet-dev
> 
>     _______________________________________________
>     servlet-dev mailing list
>     servlet-dev@xxxxxxxxxxx <mailto:servlet-dev@xxxxxxxxxxx>
>     To change your delivery options, retrieve your password, or
>     unsubscribe from this list, visit
>     https://www.eclipse.org/mailman/listinfo/servlet-dev
> 
> 
> 
> -- 
> Greg Wilkins <gregw@xxxxxxxxxxx <mailto:gregw@xxxxxxxxxxx>> CTO
> http://webtide.com
> 
> _______________________________________________
> servlet-dev mailing list
> servlet-dev@xxxxxxxxxxx
> To change your delivery options, retrieve your password, or unsubscribe from this list, visit
> https://www.eclipse.org/mailman/listinfo/servlet-dev
>