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

[Greg Wilkins <gregw@xxxxxxxxxxx>]

Mark,

While I think it's not a great idea to make error pages async, I do think there are several use cases:

• If you are writing a fully async webapp and never EVER want to block, then you need to write errors asynchronously.  Now this can be done by avoid using sendError and/or error page redirects, so that's probably not a big driver for any chance.
• Some applications do have rather complex error pages that may go through complex rendering frameworks, that may be implemented using async to assemble lots of UI components.  Of course this is fragile if the framework itself errors, but servers should have error loop detection and just render a simple page if a loop occurs.
• An app might think it is synchronous, but some async element might be mixed in.  Eg. a quality of service filter that only allows exactly as many requests past as there are JDBC connections in the connection pool. All other requests are suspended using async and resumed when a JDBC connection becomes available.   Perhaps such things should not be put on ERROR dispatches... but then naught as strange as users!

So as much as it would simplify our lives if error pages could not call startAsync, the current spec doesn't say that and I don't really think we can break the <0.1% > 0% of apps that may use this without good reason.

More responses in line:

On Fri, 5 Jul 2019 at 18:32, Mark Thomas <markt@xxxxxxxxxx> wrote:

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

--

Greg Wilkins <gregw@xxxxxxxxxxx> CTO http://webtide.com