| Re: [servlet-dev] Async sendError with error pages? |
Stuart,Cancelling sync operations is really hard... Perhaps impossible to do without significant performance impacts. I think operations like sendError are essentially writes so can't be called until isReady is true. Only then would it be possible to consider switching back to blocking mode. But do we just discard the Write Listener ?On Wed., 10 Jul. 2019, 01:41 Stuart Douglas, <sdouglas@xxxxxxxxxx> wrote:_______________________________________________On Wed, 10 Jul 2019 at 02:31, Greg Wilkins <gregw@xxxxxxxxxxx> wrote:Mark,good idea to review!Another thing to consider if you are looking at unhandled exceptions is that now with HTTP/2 it is possible to detect errors while an async request is waiting. We started off by calling AsyncListener.onError when we detected such an exception (Stream reset, channel closed etc.) but found that we broke some applications who were not expecting it to be called. So now we make that behaviour optional.I guess it would be possible for a webapp to contain an error page mapping for an exception that was asynchronously detected in HTTP/2... but it is rather tortuous path to see how that could end up in an ERROR dispatch.Of course there is the whole async IO issue as well.... once you start async IO, there really is no way to get back to synchronous IO in order to be able to call any error page mappings!What do you think about specifying that if you dispatch an async request if the stream has not been closed it reverts back to blocking mode? I just had a look in the Undertow code and we do not handle this well at all (there is basically just a TODO wondering what the correct behaviour is).Stuart_______________________________________________cheersOn Tue, 9 Jul 2019 at 17:12, Mark Thomas <markt@xxxxxxxxxx> wrote:On 09/07/2019 14:15, Greg Wilkins wrote:
> Mark, Stuart, et al,
Greg,
There are enough edge cases and moving parts here that I want to run
some tests to confirm exactly what happens in some cases. I'll update
this thread when I have some concrete information.
Another factor to bring in to this is the unhandled exception.
Mark
>
> I think that jetty will change to follow the implementations of Tomcat
> and Undertow. Which if I understand correctly is as follows for an
> non-async caller:
>
> * Thread A Normal REQUEST dispatch
> * Thread A calls sendError (state changed, output closed, but response
> not committed)
> * Thread A returns from REQUEST dispatch
> * Thread A output reopened, ERROR dispatch.
> * Thread A generates error page (perhaps calling startAsync and an
> extended lifecycle).
>
> But we'd also like to handle async callers of sendError. Do your
> containers handling it like this?:
>
> * Thread A Normal REQUEST dispatch
> * Thread A calls startAsync
> * Thread B calls sendError (state changed, output closed, but
> response not committed)
> * Winner of race between Thread A returning from REQUEST dispatch and
> Thread B calling complete() does noop
> * Loser of race reopens output and does ERROR dispatch
> * Thread A/B generates page (perhaps calling startAsync and an
> extended lifecycle).
> * Thread A/B calls onComplete when ERROR dispatch returns or second
> async lifecycle completes.
>
> I think that works for an async thread calling sendError then complete,
> but there are some edge cases that need to be considered. What if it
> calls sendError and then AsyncContext.dispatch? What if it calls
> sendError and then times out?
>
> I think that sendError followed by AsyncContext.dispatch should be
> handled with an ISE, just like trying to use a normal dispatcher would
> if sendError had been called.
>
> I'm not sure at all what to do on the timeout. If there is no
> onTimeout listener, then we'd do a ERROR dispatch for the timeout, so
> perhaps we do the same:
>
> * we call any onTimeout listeners, if they call dispatch, then it is
> ISE. If they call complete then we have our complete call and
> proceed as above.
> * If there are no onTimeout listeners or if they don't call complete,
> then we just do the ERROR dispatch for the sendError rather than for
> the timeout?
>
>
> Thoughts?
>
>
> On Mon, 8 Jul 2019 at 04:42, Stuart Douglas <sdouglas@xxxxxxxxxx
> <mailto:sdouglas@xxxxxxxxxx>> wrote:
>
>
>
> On Sat, 6 Jul 2019 at 02:33, Mark Thomas <markt@xxxxxxxxxx
> <mailto:markt@xxxxxxxxxx>> wrote:
>
> 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.
>
>
> Undertow is the same.
>
>
> > *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.
>
>
> Undertow also does the same, when sendError is called the currently
> running request return to the container before actually doing the
> error 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.
>
>
> +1
>
>
> 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.
>
>
> I don't know if it is really in line with the spec but we just
> implicitly call complete() here when the error dispatch is done.
>
>
>
> > *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 would also throw an ISE.
>
>
>
> > 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?
>
>
> I don't really like this. My gut feeling is that that sendError
> should work like javax.servlet.AsyncContext#dispatch, and basically
> finish the current async cycle. If the error page wants to do its
> own async processing then it can just call startAsync again.
>
> Stuart
>
>
>
> Mark
> _______________________________________________
> servlet-dev mailing list
> servlet-dev@xxxxxxxxxxx <mailto:servlet-dev@xxxxxxxxxxx>
> To change your delivery options, retrieve your password, or
> unsubscribe from this list, visit
> https://www.eclipse.org/mailman/listinfo/servlet-dev
>
> _______________________________________________
> servlet-dev mailing list
> servlet-dev@xxxxxxxxxxx <mailto:servlet-dev@xxxxxxxxxxx>
> To change your delivery options, retrieve your password, or
> unsubscribe from this list, visit
> https://www.eclipse.org/mailman/listinfo/servlet-dev
>
>
>
> --
> Greg Wilkins <gregw@xxxxxxxxxxx <mailto:gregw@xxxxxxxxxxx>> CTO
> http://webtide.com
>
> _______________________________________________
> servlet-dev mailing list
> servlet-dev@xxxxxxxxxxx
> To change your delivery options, retrieve your password, or unsubscribe from this list, visit
> https://www.eclipse.org/mailman/listinfo/servlet-dev
>
_______________________________________________
servlet-dev mailing list
servlet-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://www.eclipse.org/mailman/listinfo/servlet-dev
--
servlet-dev mailing list
servlet-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://www.eclipse.org/mailman/listinfo/servlet-dev
servlet-dev mailing list
servlet-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://www.eclipse.org/mailman/listinfo/servlet-dev