Yeah, I am not keen on the notes table approach either. My original patch
to fix this didn't do that. Not that my patch was great either.
The problem with calling ap_set_keepalive() multiple times is that the
function doesn't just set the keepalive flag, it also increments the
connection's keepalive counter on the call.
And the reason we are even messing with this is because we need to check
if we are on a keepalive connection in ap_die() so we know whether to
bother reading the rest of a request. Normally ap_set_keepalive() isn't
called until we send the response headers which is too late in the game
for the ap_die() case.
-Rasmus
On Fri, 27 Aug 2004, Roy T. Fielding wrote:
> This doesn't look right. Checking the notes table is a serious
> performance hit, and why does it matter how many times keepalives
> is incremented on an error path? There must be a better way to do this.
>
> ....Roy
>
>
> On Friday, August 27, 2004, at 04:44 PM, [EMAIL PROTECTED] wrote:
>
> > jim 2004/08/27 16:44:42
> >
> > Modified: src CHANGES
> > src/main http_protocol.c http_request.c
> > Log:
> > Make ap_set_keepalive more statefully aware, allowing it
> > to be called multiple times (to correctly set keepalive)
> > but not increment keepalives when not needed. This allows
> > us to handle a special case where we need to discard
> > body content "early"
> >
> > Revision Changes Path
> > 1.1949 +3 -2 apache-1.3/src/CHANGES
> >
> > Index: CHANGES
> > ===================================================================
> > RCS file: /home/cvs/apache-1.3/src/CHANGES,v
> > retrieving revision 1.1948
> > retrieving revision 1.1949
> > diff -u -r1.1948 -r1.1949
> > --- CHANGES 27 Aug 2004 19:29:57 -0000 1.1948
> > +++ CHANGES 27 Aug 2004 23:44:41 -0000 1.1949
> > @@ -24,9 +24,10 @@
> > was not checked properly. This affects mod_usertrack and
> > core. PR 28218. [André Malo]
> >
> > - *) No longer breaks mod_dav, frontpage and others. Backs out
> > - a patch which prevented discarding the request body for
> > requests
> > + *) No longer breaks mod_dav, frontpage and others. Repair a patch
> > + in 1.3.31 which prevented discarding the request body for
> > requests
> > that will be keptalive but are not currently keptalive. PR
> > 29237.
> > + [Jim Jagielski]
> >
> > *) COMPATIBILITY: Added new compile-time flag:
> > UCN_OFF_HONOR_PHYSICAL_PORT.
> > It controls how UseCanonicalName Off determines the port value
> > if
> >
> >
> >
> > 1.336 +12 -1 apache-1.3/src/main/http_protocol.c
> >
> > Index: http_protocol.c
> > ===================================================================
> > RCS file: /home/cvs/apache-1.3/src/main/http_protocol.c,v
> > retrieving revision 1.335
> > retrieving revision 1.336
> > diff -u -r1.335 -r1.336
> > --- http_protocol.c 15 Apr 2004 15:51:51 -0000 1.335
> > +++ http_protocol.c 27 Aug 2004 23:44:41 -0000 1.336
> > @@ -391,6 +391,7 @@
> > int wimpy = ap_find_token(r->pool,
> > ap_table_get(r->headers_out,
> > "Connection"), "close");
> > const char *conn = ap_table_get(r->headers_in, "Connection");
> > + const char *herebefore = ap_table_get(r->notes,
> > "ap_set_keepalive-called");
> >
> > /* The following convoluted conditional determines whether or
> > not
> > * the current connection should remain persistent after this
> > response
> > @@ -442,7 +443,17 @@
> > int left = r->server->keep_alive_max -
> > r->connection->keepalives;
> >
> > r->connection->keepalive = 1;
> > - r->connection->keepalives++;
> > + /*
> > + * ap_set_keepalive could be called multiple times (eg: in
> > + * ap_die() followed by ap_send_http_header()) during this
> > + * one single request. To ensure that we don't incorrectly
> > + * increment the keepalives counter for each call, we
> > + * use notes to store a state flag.
> > + */
> > + if (!herebefore) {
> > + r->connection->keepalives++;
> > + ap_table_setn(r->notes, "ap_set_keepalive-called", "1");
> > + }
> >
> > /* If they sent a Keep-Alive token, send one back */
> > if (ka_sent) {
> >
> >
> >
> > 1.176 +7 -1 apache-1.3/src/main/http_request.c
> >
> > Index: http_request.c
> > ===================================================================
> > RCS file: /home/cvs/apache-1.3/src/main/http_request.c,v
> > retrieving revision 1.175
> > retrieving revision 1.176
> > diff -u -r1.175 -r1.176
> > --- http_request.c 28 May 2004 12:07:02 -0000 1.175
> > +++ http_request.c 27 Aug 2004 23:44:41 -0000 1.176
> > @@ -1051,12 +1051,18 @@
> > }
> >
> > /*
> > + * We need to ensure that r->connection->keepalive is valid in
> > + * order to determine if we can discard the request body below.
> > + */
> > + ap_set_keepalive(r);
> > +
> > + /*
> > * If we want to keep the connection, be sure that the request
> > body
> > * (if any) has been read.
> > */
> > if ((r->status != HTTP_NOT_MODIFIED) && (r->status !=
> > HTTP_NO_CONTENT)
> > && !ap_status_drops_connection(r->status)
> > - && r->connection && (r->connection->keepalive != -1)) {
> > + && r->connection && (r->connection->keepalive > 0)) {
> >
> > (void) ap_discard_request_body(r);
> > }
> >
> >
> >
> >
> >
>
>
