Received: by taz.hyperreal.com (8.8.4/V2.0) id MAA13478; Wed, 12 Mar 1997 12:33:32 -0800 (PST) Received: from twinlark.arctic.org by taz.hyperreal.com (8.8.4/V2.0) with SMTP id MAA13419; Wed, 12 Mar 1997 12:33:24 -0800 (PST) Received: (qmail 31197 invoked by uid 500); 12 Mar 1997 20:33:44 -0000 Date: Wed, 12 Mar 1997 12:33:44 -0800 (PST) From: Dean Gaudet To: new-httpd@hyperreal.com Subject: Re: [PATCH]: "Undying keepalives in 1.2b7" on Solaris 2.x (fwd) In-Reply-To: <9703110310.aa20158@paris.ics.uci.edu> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: new-httpd-owner@apache.org Precedence: bulk Reply-To: new-httpd@hyperreal.com On reading rfc2068 it isn't clear to me that a Transfer-Coding is *required* to provide "safe transport" such as that required by keepalive... It's strongly hinted at but I didn't see a MUST or a SHOULD. Other than that this looks good on first read, but I want to give it another read/walk before I +1. Dean On Tue, 11 Mar 1997, Roy T. Fielding wrote: > Dean Gaudet writes: > >Why don't we just move the nokeepalive test into set_keepalive? > > Ummm, because that would be far too sensible? > > >So here's a patch that moves the nokeepalive test into set_keepalive. > > Oh great, that made me look hard at the big conditional again and > I found a couple more bugs having to do with the ordering of tests. > So, here's a new patch to stomp them out and make it a little > more understandable. [Yes, it is a bigger change, and yes it must > be fixed before the next beta, let alone 1.2 final.] > > .....Roy > > Index: http_protocol.c > =================================================================== > RCS file: /export/home/cvs/apache/src/http_protocol.c,v > retrieving revision 1.107 > diff -c -r1.107 http_protocol.c > *** http_protocol.c 1997/03/07 14:43:52 1.107 > --- http_protocol.c 1997/03/11 11:05:12 > *************** > *** 255,281 **** > > int set_keepalive(request_rec *r) > { > ! char *conn = table_get(r->headers_in, "Connection"); > ! char *length = table_get(r->headers_out, "Content-Length"); > ! char *tenc = table_get(r->headers_out, "Transfer-Encoding"); > ! int ka_sent; > ! > ! if (r->connection->keepalive == -1) /* Did we get bad input? */ > ! r->connection->keepalive = 0; > ! else if (r->server->keep_alive && (!r->server->keep_alive_max || > ! (r->server->keep_alive_max > r->connection->keepalives)) && > ! (r->server->keep_alive_timeout > 0) && > ! (r->status == HTTP_NOT_MODIFIED || r->status == HTTP_NO_CONTENT > ! || r->header_only || length || tenc || > ! ((r->proto_num >= 1001) && (r->chunked = 1))) && > ! (!find_token(r->pool, conn, "close")) && > ! ((ka_sent = find_token(r->pool, conn, "keep-alive")) || > ! r->proto_num >= 1001)) { > ! /* > ! * The (r->chunked = 1) in the above expression is a side-effect > ! * that sets the output to chunked encoding if it is not already > ! * length-delimited. It is not a bug, though it is annoying. > ! */ > char header[256]; > int left = r->server->keep_alive_max - r->connection->keepalives; > > --- 255,301 ---- > > int set_keepalive(request_rec *r) > { > ! char *conn = table_get(r->headers_in, "Connection"); > ! int ka_sent = 0; > ! > ! /* The following convoluted conditional determines whether or not > ! * the current connection should remain persistent after this response > ! * (a.k.a. HTTP Keep-Alive) and whether or not the output message > ! * body should use the HTTP/1.1 chunked transfer-coding. In English, > ! * > ! * IF we have not marked this connection as errored; > ! * and the message body has a defined length due to the status code > ! * being 304 or 204, the request method being HEAD, the > ! * response having defined Content-Length or Transfer-Encoding, > ! * or the request version being HTTP/1.1 and thus capable of being > ! * set as chunked [we know the (r->chunked = 1) side-effect is ugly]; > ! * and the server configuration enables keep-alive; > ! * and the server configuration has a reasonable inter-request timeout; > ! * and there is no maximum # requests or the max hasn't been reached; > ! * and the client did not request non-persistence (Connection: close); > ! * and the client is requesting an HTTP/1.0-style keep-alive > ! * and we haven't been configured to ignore the buggy twit, > ! * or the client claims to be HTTP/1.1 compliant (perhaps a proxy); > ! * THEN we can be persistent, which requires more headers be output. > ! * > ! * Note that the condition evaluation order is extremely important. > ! */ > ! if ((r->connection->keepalive != -1) && > ! ((r->status == HTTP_NOT_MODIFIED) || > ! (r->status == HTTP_NO_CONTENT) || > ! r->header_only || > ! table_get(r->headers_out, "Content-Length") || > ! table_get(r->headers_out, "Transfer-Encoding") || > ! ((r->proto_num >= 1001) && (r->chunked = 1))) && > ! r->server->keep_alive && > ! (r->server->keep_alive_timeout > 0) && > ! ((r->server->keep_alive_max == 0) || > ! (r->server->keep_alive_max > r->connection->keepalives)) && > ! !find_token(r->pool, conn, "close") && > ! (((ka_sent = find_token(r->pool, conn, "keep-alive")) && > ! !table_get(r->subprocess_env, "nokeepalive")) || > ! (r->proto_num >= 1001)) > ! ) { > char header[256]; > int left = r->server->keep_alive_max - r->connection->keepalives; > > *************** > *** 297,309 **** > return 1; > } > > ! /* We only really need to send this to HTTP/1.1 clients, but we > * always send it anyway, because a broken proxy may identify itself > * as HTTP/1.0, but pass our request along with our HTTP/1.1 tag > * to a HTTP/1.1 client. Better safe than sorry. > */ > rputs("Connection: close\015\012", r); > > return 0; > } > > --- 317,334 ---- > return 1; > } > > ! /* Otherwise, we need to indicate that we will be closing this > ! * connection immediately after the current response. > ! * > ! * We only really need to send "close" to HTTP/1.1 clients, but we > * always send it anyway, because a broken proxy may identify itself > * as HTTP/1.0, but pass our request along with our HTTP/1.1 tag > * to a HTTP/1.1 client. Better safe than sorry. > */ > rputs("Connection: close\015\012", r); > > + r->connection->keepalive = 0; > + > return 0; > } > > *************** > *** 1116,1123 **** > > basic_http_header (r); > > ! if (!table_get(r->subprocess_env, "nokeepalive")) > ! set_keepalive (r); > > if (r->chunked) { > bputs("Transfer-Encoding: chunked\015\012", fd); > --- 1141,1147 ---- > > basic_http_header (r); > > ! set_keepalive (r); > > if (r->chunked) { > bputs("Transfer-Encoding: chunked\015\012", fd); >