Mark, Stuart, et al,
I'm not sold on the infinite loop issue, because if we don't set a timeout
the startAsync cycle will go forever anyway.
However, I can see the argument that after an onTimeout event, you really
need a new dispatch and startAsync cycle to create the error page, as it is
likely (or at least possible) that the error page is generated via
different framework etc. Having the error page independent is much
better than trying to pivot to async error page generation in an onTimeout
or onError callback.
However, we still don't have perfect isolation between the initial page
generation attempt and the error page dispatch, because once an async
WriteListener is registered, there is no way to unregister it! So the
error page would have to be generated asynchronously and by the same
WriteListener - as we are not allowed to set it to null nor set it to a new
listener.
So if we wish to consider the error dispatch as an entirely different
cycle, then we need the ability to serve the error page blocking or async,
and if it is async then it should be able to set it's own WriteListener.
So perhaps any read/write listeners that are set are unset after onError or
onTimeout callbacks return?
thoughts?
On 1 November 2016 at 08:44, Stuart Douglas <sdouglas_at_redhat.com> wrote:
> Thinking about this a bit more I think that the extra dispatch() +
> startAsync() that is possible today is actually an advantage.
>
> A new call to startAsync() means new listeners, so it is relatively
> obvious that the listener to dispatch to the error page should not be
> added to the error page itself, while with what Greg is proposing you
> would need some kind of guard in the listener to make sure that if
> onTimeout is called a second time the timeout is not just blindly
> updated.
>
> You could say something like 'if onTimeout is called and the timout is
> updated all listeners are cleared from the AsyncContext' but I think
> that is getting very unintuitive.
>
> Stuart
>
> On Tue, Nov 1, 2016 at 1:59 AM, Mark Thomas <markt_at_apache.org> wrote:
> > On 27/10/2016 06:53, Greg Wilkins wrote:
> >> All,
> >> we've just realized that the way the AsyncListener onTimeout (and
> >> probably onError) method(s) are specified precludes an implementation
> >> from writing a response asynchronously.
> >>
> >> The AsyncContext javadoc says:
> >>
> >> In the event that an asynchronous operation has timed out, the
> >> container must run through these steps:
> >> 1. Invoke, at their onTimeout method, all AsyncListener instances
> >> registered with the ServletRequest on which the asynchronous
> >> operation was initiated.
> >> 2. If none of the listeners called complete or any of the dispatch
> >> methods, perform an error dispatch with a status code equal to
> >> HttpServletResponse.SC_INTERNAL_SERVER_ERROR.
> >> 3. If no matching error page was found, or the error page did not
> >> call complete or any of the dispatch methods, call complete.
> >>
> >>
> >>
> >> So once onTimeout is called, the implementation can only complete
> >> (presumably after writing a blocking response) or dispatch, which would
> >> then have to do another startAsync cycle if the response was to be
> >> written using async IO.
> >>
> >> I'm wondering if we should allow AsyncContext.setTimeout to be called to
> >> indicate that the current async cycle should continue with the new
> >> timeout. This would allow an response to be written asynchronously.
> >
> > Wouldn't that make it very easy for an app to enter an infinite loop? It
> > strikes me as wrong for the API to make it that easy to mess up that
> > badly. Yes, you can still do this with the current API but you have to
> > try a lot harder.
> >
> > Mark
> >
>
--
Greg Wilkins <gregw@webtide.com> CTO http://webtide.com