On 05/07/2019 10:28, Greg Wilkins wrote:
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.
See above.
I'm not saying we shouldn't support async error pages but it does seem
... odd.
Yep. And considering the bugs we've had... pretty rare. But bugs to get reported, so some folks are using async error pages.
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()
Ah, so you are essentially using the async dispatch mechanism all the time, but the resulting dispatch is ERROR rather than ASYNC.
Nice idea! Hmmm we may consider doing similar in our next major release.
But I don't think it solves all the problems....
> *Non async caller with async error page.*
> [...]
Tomcat avoids this race because of when it performs the dispatch.
Nice!
> *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.
Agreed. Perhaps to extend the tomcat do-error-dispatch-after-request-is-complete approach, we
could actually wait until the async processing called complete and then do the error dispatch!
I think the calling thread should call complete().
Definitely agree! The behaviour withing sendError may be beyond their control, so how do they know if they shouldn't
What is the dispatch target's justification for calling complete()? That
seems wrong to me so an ISE looks reasonable.
Dispatch target should not call complete. Our current fix is to use a real async dispatch call, which ends the async lifecycle and
makes the sendError callers complete an ISE. But if we wait until it calls complete and then do the ERROR dispatch, I think that works.
> *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.
I think it is legal, as we can't stop an ERROR dispatch calling startAsync.
But again, I think the delayed ERROR dispatch until after complete is called solves this one as well, because the error dispatch is
not happening inside the sendError call.
I think being more specific would be a good thing.
Well I think if Tomcats delayed ERROR dispatch is extended to happen after an async complete call, the perhaps we don't need to be
more specific... the vague javadoc language works for users and implementers just have to be smart (or borrow ideas from smart people!)
Would stating that targets of ERROR dispatches must be synchronous a
possible way forward?
Again, I don't think we can.
tl;dr; If the current spec allows the tomcat behaviour of delaying ERROR dispatch until after the current request lifecycle completes, then I think we can implement error pages the way the spec vaguely describes.
cheers