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

[Mark Thomas <markt@xxxxxxxxxx>]

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.

> *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.

> *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.

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.

> *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 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?

Mark