On Tue, Nov 1, 2016 at 5:18 PM, Greg Wilkins <gregw_at_webtide.com> wrote:
>
> 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.
I'm not sure what you mean by this?
>
> 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?
We actually already have this issue, as it stands if you have set a
write listener and then dispatch back to the container you are in a
weird situation where the async cycle is complete, however the write
listener is still set on the output stream. I think it would make more
sense to clear the read/write listener on dispatch(), rather than
onError or onTimeout.
That said from the error page point of view it seems like a bit of a
corner case, as if you have set a write listener in most cases you
will end up with the response being committed, so you can't generate
an error page anyway.
Stuart
>
> 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