Re: WICKET-5241 gives pause to think about our request cycle

[Andrea Del Bene]

I think Martin had created an issue about it some weeks ago...

Hi,
I think we could leverage async support built-in in Servlet 3.x. To prevent
developers doing long tasks in the detach method, I would suggest an error
log trace telling the problem.

Regards,

__
Cedric Gatay (@Cedric_Gatay <http://twitter.com/Cedric_Gatay>)
http://code-troopers.com | http://www.bloggure.info | http://cedric.gatay.fr


On Tue, Jun 25, 2013 at 1:45 PM, Martijn Dashorst <
martijn.dasho...@gmail.com> wrote:

The issue reported in
https://issues.apache.org/jira/browse/WICKET-5241 gives pause to two
things:

1. The response is not written to the client until the detach has been
performed

2. In wicket 1.5 and up the detach phase is done twice

Ad 1:

It has been my assumption (apparently wrongly so) that the detach
phase would run after we sent the response to the client, and that we
were unable to report any detach errors to the client due to this. In
Wicket 6 (probably 5 as well) the filter calls
RequestCycle#processRequestAndDetach(), and then flushes the response
to the client. This lets all browsers pause until after the detach
phase.

The courtesy flush

In my opinion we should flush first, making life better for our
clients, and then clean up after us (detach the used objects).

The downside

I'm not 100% sure if this goes well when you have a long lasting
detach, say 10 seconds or so, and a client sends a new request to the
page, or when the page is stateful, and wicket sends a redirect.

Here's an improved WicketFilter#processRequestCycle implementation
that flushes prior to detaching:

         protected boolean processRequestCycle(RequestCycle requestCycle,
                         WebResponse webResponse, HttpServletRequest
httpServletRequest,
                         HttpServletResponse httpServletResponse, final
FilterChain chain)
                         throws IOException, ServletException {
                 // Assume we are able to handle the request
                 boolean requestHandledByWicket = true;

                 try {
                         requestHandledByWicket =
requestCycle.processRequest();
                         if (requestHandledByWicket) {
                                 webResponse.flush();
                         }
                 } finally {
                         requestCycle.detach();
                 }

                 if (!requestHandledByWicket && chain != null) {
                         chain.doFilter(httpServletRequest,
httpServletResponse);
                 }
                 return requestHandledByWicket;
         }

Unfortunately, this doesn't work for stateful pages, as they redirect
directly to a buffer. With curl, the request is blocked on the socket
(the connection is reused) making the request block until the detach
has passed. This is also true for at least safari. If the connection
is not reused (new curl request to the stateful redirect url), then
processing can continue as expected.

With servlet 3 we could potentially do the request asynchronously, not
blocking the socket anymore during detach, with the additional caveat
that newly incoming requests (e.g. a link click) to the detaching page
would still have to wait for the lock to be released—you don't want to
start working with a still detaching page... Note that this is only
relevant for detach cycles that take longer than say 100ms. Anything
shorter would only be noticeable in artificial test scenarios I
imagine.

What are your thoughts?

Martijn