Bug 52593 - outputbuffer.recycle() is called immediately after response instead of at socket close
Summary: outputbuffer.recycle() is called immediately after response instead of at soc...
Status: RESOLVED WONTFIX
Alias: None
Product: Tomcat 7
Classification: Unclassified
Component: Connectors (show other bugs)
Version: 7.0.25
Hardware: All All
: P2 normal (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on: 52547
Blocks:
  Show dependency tree
 
Reported: 2012-02-03 17:57 UTC by David
Modified: 2012-02-06 11:02 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description David 2012-02-03 17:57:47 UTC
+++ This bug was initially created as a clone of Bug #52547 +++

The originally described bug #52547 is fixed.

But there is a related bug described in my 2nd comment
https://issues.apache.org/bugzilla/show_bug.cgi?id=52547#c2

--------- copy below ------------

Additionally, I think there is a "bug" in that the recycle() happens
immediatley after the nextRequest() even when keepalive is true (I can see from
an iptables log that the socket is not closed for another default 20 seconds).

See the event(...), asyncDispatch(...) and service(...) methods in
http://svn.apache.org/repos/asf/tomcat/tc7.0.x/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java

Basically response.recycle() (when called because not dealing with async part
of the request) calls outputBuffer.recycle() immediatley response processing is
finished rather than just before the socket is actually closed.

So in an http/1.1 multi request-response (without a socket close) you still get
both outputBuffer.nextRequest() and outputBuffer.recycle() called consecutvely
at the end of each completed response process.

I can see that nextRequest(), recycle() should both exist, but only if
recycle() really only happens at socket close as it's descriptive comment says.
Otherwise it would be a (very minor) optimization to remove the (nearlly always
present) duplicate calls.
Comment 1 Mark Thomas 2012-02-05 20:48:23 UTC
It is this way for several reasons:
- support for pipelined requests
- ensure correct recycling of objects after an error
- simpler code

Correctly determining when recycled() is required and isn't would require more complexity to monitor connection state than the extra recycles currently used.

Further, the impact of not recycling is far worse than the impact of extra recycling.

If there was a case where there were multiple recycle() calls and it was clear that one or more was always unnecessary then there would be a case to remove it. That is not the case here.
Comment 2 David 2012-02-06 09:19:27 UTC
Just to point out - taking into account all these things - it would be simpler (a micro optimization but clearer code) to combine outputbuffer.nextRequest() & recycle() and always call one of them just once at the end of response processing.
Comment 3 Mark Thomas 2012-02-06 10:03:37 UTC
No it wouldn't.

recycle() != nextRequest() and to support HTTP pipelining this difference is critical.

This code is not going to be changed.
Comment 4 David 2012-02-06 11:02:26 UTC
OK and thanks for the reply.

I can't get my head around the pipelining as I'm not using it.

So I'll defer to your greater knowledge.