Bug 367716

Comment 1 Thomas Becker 2012-01-03 12:56:45 EST

There's no maxIdleTimeout Tests for suspended asyn http requests. 

Add tests for maxIdleTimeout after a request has been suspended+resumed, suspended+timeout, suspended+complete.

Doing this, I found out that maxIdleTimeout doesn't get reenabled after a request has been suspended. 
It gets disabled in AsyncHttpConnection line 59:

            // don't check for idle while dispatched (unless blocking IO is done).
            _asyncEndp.setCheckForIdle(false);

And needs to be reenabled after resume/timeout/complete. I will fix it as part of this issue.

Comment 2 Thomas Becker 2012-01-04 11:17:34 EST

Created attachment 209021 [details]
proposed patch

Patch attached. 

Fixes maxIdleTime after suspend/resume/complete and adds tests for each case.

Comment 3 Thomas Becker 2012-01-05 07:19:28 EST

Created attachment 209065 [details]
proposed patch - second commit

Comment 4 Simone Bordet 2012-01-05 12:36:03 EST

Reviewed and fixed: the 2 patches implement the right behavior and add tests to prove it.

Comment 5 Greg Wilkins 2012-01-07 17:34:49 EST

Can you add a comment to the code to explain the line


      if (_request.getAsyncContinuation().isComplete() || _request.getAsyncContinuation().isInitial())
      {
         _asyncEndp.setCheckForIdle(true);
      }


I would expect that we should re-enable idle for all exits from handle except if the request is suspended (in which case the suspended timeout is monitoring the connection).   Do you agree with that logic?

If so, it would be best to rewrite that condition so that it looks like that logic.  Either way, we need a comment describing the logic.

Comment 6 Greg Wilkins 2012-01-08 19:16:41 EST

Reviewing this again I see that the logic is using the initial state to determine if the request is suspended, and then excluding the completed state from not re-initializing the idle check. This is too convoluted.

I think the code should just be:
 
            // reenable idle checking unless request is suspended
            if(!_request.getAsyncContinuation().isAsyncStarted())
            {
                _asyncEndp.setCheckForIdle(true);
            }

This passes the jetty-server tests, so I've committed.  Please review and if you agree then mark issue as resolved.

Comment 7 Thomas Becker 2012-01-09 04:13:47 EST

Hi Greg,

reviewed your changes, retestet them and they're good. Like the new if/else in the finally block. The new maxIdleTests are still green.

I will resolve this issue now. maxIdle is working again after resume/timeout/complete.

Cheers,
Thomas

Comment 8 Thomas Becker 2012-01-11 10:56:03 EST

Created attachment 209318 [details]
Enhanced tests

Better (actually working) tests.

Comment 9 Thomas Becker 2012-01-12 04:30:10 EST

Reopened as the new tests need to be commited, please.

Comment 10 Simone Bordet 2012-01-12 04:40:22 EST

Enhanced test patch has been reviewed and committed.