You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by rb...@covalent.net on 2001/04/17 06:07:02 UTC
Multi-protocol patch, part 1
Most, if not all, of you have seen the multi-protocol patch that used to
be posted at klomp.covalent.net:8080/. That web site will be back up as
soon as we can do it. Covalent has recently moved(needed more space), and
that web site was a casualty until things settle down. Should be
sometime tomorrow. In the mean-time, I have begun to split the patch into
managable pieces. This post is the first part of that work.
What this post basically does, is it breaks part of the conn_rec out and
into an HTTP specific structure. I am not saying that this is everything
that can be pulled out of the conn_rec, but it is a good first step.
Comments?
Ryan
Index: include/httpd.h
===================================================================
RCS file: /home/cvs/httpd-2.0/include/httpd.h,v
retrieving revision 1.146
diff -u -d -b -w -u -r1.146 httpd.h
--- include/httpd.h 2001/04/16 20:33:16 1.146
+++ include/httpd.h 2001/04/17 03:53:21
@@ -860,14 +860,10 @@
unsigned aborted:1;
/** Are we using HTTP Keep-Alive? -1 fatal error, 0 undecided, 1 yes */
signed int keepalive:2;
- /** Did we use HTTP Keep-Alive? */
- unsigned keptalive:1;
/** have we done double-reverse DNS? -1 yes/failure, 0 not yet,
* 1 yes/success */
signed int double_reverse:2;
- /** How many times have we used it? */
- int keepalives;
/** server IP address */
char *local_ip;
/** used for ap_get_server_name when UseCanonicalName is set to DNS
Index: modules/http/config.m4
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/http/config.m4,v
retrieving revision 1.4
diff -u -d -b -w -u -r1.4 config.m4
--- modules/http/config.m4 2001/03/04 06:27:27 1.4
+++ modules/http/config.m4 2001/04/17 03:53:22
@@ -7,5 +7,8 @@
APACHE_MODULE(http, HTTP protocol handling, $http_objects, , yes)
APACHE_MODULE(mime, mapping of file-extension to MIME, , , yes)
+if test "$enable_http" = "yes"; then
+ AC_DEFINE(AP_HTTP_ENABLED, 1, [HTTP is enabled on this server])
+fi
APACHE_MODPATH_FINISH
Index: modules/http/http_core.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/http/http_core.c,v
retrieving revision 1.270
diff -u -d -b -w -u -r1.270 http_core.c
--- modules/http/http_core.c 2001/04/11 23:37:16 1.270
+++ modules/http/http_core.c 2001/04/17 03:53:22
@@ -295,6 +295,35 @@
return OK;
}
+static int http_create_request(request_rec *r)
+{
+ http_conn_rec *conn = ap_get_module_config(r->connection->conn_config, &http_module);
+
+ conn = apr_palloc(r->pool, sizeof(*conn));
+ ap_set_module_config(r->connection->conn_config, &http_module, conn);
+
+ if (!r->main && !(r->prev || r->next)) {
+ conn->keptalive = r->connection->keepalive == 1;
+ r->connection->keepalive = 0;
+
+ apr_setsocketopt(r->connection->client_socket, APR_SO_TIMEOUT,
+ (int)(conn->keptalive
+ ? r->server->keep_alive_timeout * APR_USEC_PER_SEC
+ : r->server->timeout * APR_USEC_PER_SEC));
+
+ if (conn->keptalive) {
+ apr_setsocketopt(r->connection->client_socket,
+ APR_SO_TIMEOUT,
+ (int)(r->server->timeout * APR_USEC_PER_SEC));
+ }
+
+ conn->keptalive = 0;
+
+ return OK;
+ }
+ return OK;
+}
+
static void register_hooks(apr_pool_t *p)
{
ap_hook_pre_connection(ap_pre_http_connection,NULL,NULL,
@@ -303,6 +332,7 @@
APR_HOOK_REALLY_LAST);
ap_hook_http_method(http_method,NULL,NULL,APR_HOOK_REALLY_LAST);
ap_hook_default_port(http_port,NULL,NULL,APR_HOOK_REALLY_LAST);
+ ap_hook_create_request(http_create_request, NULL, NULL, APR_HOOK_MIDDLE);
ap_register_input_filter("HTTP_IN", ap_http_filter, AP_FTYPE_CONNECTION);
ap_register_input_filter("DECHUNK", ap_dechunk_filter, AP_FTYPE_TRANSCODE);
Index: modules/http/http_protocol.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/http/http_protocol.c,v
retrieving revision 1.313
diff -u -d -b -w -u -r1.313 http_protocol.c
--- modules/http/http_protocol.c 2001/04/16 21:16:53 1.313
+++ modules/http/http_protocol.c 2001/04/17 03:53:23
@@ -105,6 +105,7 @@
int ka_sent = 0;
int wimpy = ap_find_token(r->pool,
apr_table_get(r->headers_out, "Connection"), "close");
+ http_conn_rec *hconn = ap_get_module_config(r->connection->conn_config, &http_module);
const char *conn = apr_table_get(r->headers_in, "Connection");
/* The following convoluted conditional determines whether or not
@@ -146,7 +147,7 @@
&& r->server->keep_alive
&& (r->server->keep_alive_timeout > 0)
&& ((r->server->keep_alive_max == 0)
- || (r->server->keep_alive_max > r->connection->keepalives))
+ || (r->server->keep_alive_max > hconn->keepalives))
&& !ap_status_drops_connection(r->status)
&& !wimpy
&& !ap_find_token(r->pool, conn, "close")
@@ -154,10 +155,10 @@
|| apr_table_get(r->headers_in, "Via"))
&& ((ka_sent = ap_find_token(r->pool, conn, "keep-alive"))
|| (r->proto_num >= HTTP_VERSION(1,1)))) {
- int left = r->server->keep_alive_max - r->connection->keepalives;
+ int left = r->server->keep_alive_max - hconn->keepalives;
r->connection->keepalive = 1;
- r->connection->keepalives++;
+ hconn->keepalives++;
/* If they sent a Keep-Alive token, send one back */
if (ka_sent) {
Index: modules/http/mod_core.h
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/http/mod_core.h,v
retrieving revision 1.6
diff -u -d -b -w -u -r1.6 mod_core.h
--- modules/http/mod_core.h 2001/02/28 15:24:07 1.6
+++ modules/http/mod_core.h 2001/04/17 03:53:23
@@ -70,6 +70,15 @@
* @package mod_core private header file
*/
+typedef struct http_conn_rec http_conn_rec;
+
+struct http_conn_rec {
+ /** Did we use HTTP Keep-Alive? */
+ unsigned keptalive:1;
+ /** How many times have we used it? */
+ int keepalives;
+};
+
/*
* These (input) filters are internal to the mod_core operation.
*/
@@ -91,6 +100,8 @@
AP_DECLARE(int) ap_send_http_trace(request_rec *r);
int ap_send_http_options(request_rec *r);
+
+AP_DECLARE_DATA module http_module;
#ifdef __cplusplus
}
Index: modules/loggers/mod_log_config.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/loggers/mod_log_config.c,v
retrieving revision 1.52
diff -u -d -b -w -u -r1.52 mod_log_config.c
--- modules/loggers/mod_log_config.c 2001/03/09 20:30:32 1.52
+++ modules/loggers/mod_log_config.c 2001/04/17 03:53:23
@@ -191,6 +191,7 @@
#include "http_core.h" /* For REMOTE_NAME */
#include "http_log.h"
#include "http_protocol.h"
+#include "mod_core.h"
#if APR_HAVE_UNISTD_H
#include <unistd.h>
@@ -527,13 +528,19 @@
}
static const char *log_connection_status(request_rec *r, char *a)
{
+#ifdef AP_HTTP_ENABLED
+ http_conn_rec *hconn = ap_get_module_config(r->connection->conn_config,
+ &http_module);
+#endif
if (r->connection->aborted)
return "X";
+#ifdef AP_HTTP_ENABLED
if ((r->connection->keepalive) &&
- ((r->server->keep_alive_max - r->connection->keepalives) > 0)) {
+ ((r->server->keep_alive_max - hconn->keepalives) > 0)) {
return "+";
}
+#endif
return "-";
}
Index: server/protocol.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/protocol.c,v
retrieving revision 1.12
diff -u -d -b -w -u -r1.12 protocol.c
--- server/protocol.c 2001/04/11 23:37:16 1.12
+++ server/protocol.c 2001/04/17 03:53:26
@@ -556,9 +556,6 @@
r->connection = conn;
r->server = conn->base_server;
- conn->keptalive = conn->keepalive == 1;
- conn->keepalive = 0;
-
r->user = NULL;
r->ap_auth_type = NULL;
@@ -584,11 +581,6 @@
r->output_filters = conn->output_filters;
r->input_filters = conn->input_filters;
- apr_setsocketopt(conn->client_socket, APR_SO_TIMEOUT,
- (int)(conn->keptalive
- ? r->server->keep_alive_timeout * APR_USEC_PER_SEC
- : r->server->timeout * APR_USEC_PER_SEC));
-
ap_add_output_filter("BYTERANGE", NULL, r, r->connection);
ap_add_output_filter("CONTENT_LENGTH", NULL, r, r->connection);
ap_add_output_filter("HTTP_HEADER", NULL, r, r->connection);
@@ -604,10 +596,6 @@
}
return NULL;
}
- if (r->connection->keptalive) {
- apr_setsocketopt(r->connection->client_socket, APR_SO_TIMEOUT,
- (int)(r->server->timeout * APR_USEC_PER_SEC));
- }
if (!r->assbackwards) {
get_mime_headers(r);
if (r->status != HTTP_REQUEST_TIME_OUT) {
@@ -645,8 +633,6 @@
/* we may have switched to another server */
r->per_dir_config = r->server->lookup_defaults;
-
- conn->keptalive = 0; /* We now have a request to play with */
if ((!r->hostname && (r->proto_num >= HTTP_VERSION(1,1))) ||
((r->proto_num == HTTP_VERSION(1,1)) &&
Re: Multi-protocol patch, part 1
Posted by rb...@covalent.net.
On Tue, 17 Apr 2001, Greg Stein wrote:
> On Tue, Apr 17, 2001 at 08:34:07AM -0700, rbb@covalent.net wrote:
> >...
> > > Take a look at server/protocol.c, where this stuff came from. In between the
> > > two sets of the timeout, you'll see it goes to the socket to read the
> > > request line. Basically, it sets the timeout to <something>, reads a line,
> > > then resets it to <something>.
> > >
> > > In the above code, it simply sets it a couple times and exits. Not very
> > > effective :-)
> >
> > I'm fixing this now. I'll repost when I am done. This is part of the
> > problem with splitting a large patch into a couple of small patches. Not
>
> I hear ya. But I never saw this in the big patch, so having the smaller
> patch is, therefore, quite a decent payback.
>
> > that I am convinced it works correctly in the large patch, but I know I
> > tackled this exact bug back when I first wrote the large patch. Expect
> > the new patch later today.
>
> I just realized one approach to fixing this.
>
> The timeout surrounds read_request_line(). Well, that function is HTTP
> specific. (not all protos read a "request line" as their first order of
> business, and certainly not an HTTP request line)
>
> Maybe you could find a way to move the timeout setting + read_request_line
> to mod_http? As a bundle, this would work.
That's exactly what I did. I am actually re-gen'ing the patch right now,
so I should have it ready for review in a few minutes.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: Multi-protocol patch, part 1
Posted by Greg Stein <gs...@lyra.org>.
On Tue, Apr 17, 2001 at 08:34:07AM -0700, rbb@covalent.net wrote:
>...
> > Take a look at server/protocol.c, where this stuff came from. In between the
> > two sets of the timeout, you'll see it goes to the socket to read the
> > request line. Basically, it sets the timeout to <something>, reads a line,
> > then resets it to <something>.
> >
> > In the above code, it simply sets it a couple times and exits. Not very
> > effective :-)
>
> I'm fixing this now. I'll repost when I am done. This is part of the
> problem with splitting a large patch into a couple of small patches. Not
I hear ya. But I never saw this in the big patch, so having the smaller
patch is, therefore, quite a decent payback.
> that I am convinced it works correctly in the large patch, but I know I
> tackled this exact bug back when I first wrote the large patch. Expect
> the new patch later today.
I just realized one approach to fixing this.
The timeout surrounds read_request_line(). Well, that function is HTTP
specific. (not all protos read a "request line" as their first order of
business, and certainly not an HTTP request line)
Maybe you could find a way to move the timeout setting + read_request_line
to mod_http? As a bundle, this would work.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: Multi-protocol patch, part 1
Posted by Greg Stein <gs...@lyra.org>.
On Tue, Apr 17, 2001 at 01:21:55PM -0400, Bill Stoddard wrote:
> >
> > > However, please note that we can/should simply toss the darned bit fields
> > > from conn_rec, and certainly this one.
> >
> > I seriously disagree with this. bit fields are incredibly useful, and we
> > should take advantage of them, not dump them.
> >
>
> FWIW, I am not a fan of bit fields. They are slower to check and all you get back is a bit of space
> savings....maybe (if you cram multiple bit fields into an int).
Exactly.
Even worse was the case where I said to toss the bitfield:
int whatever:1;
int something_else;
That bitfield isn't very useful :-)
[ but rather moot since the "whatever" isn't even needed; it becomes a local
variable ]
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: Multi-protocol patch, part 1
Posted by Bill Stoddard <bi...@wstoddard.com>.
>
> > However, please note that we can/should simply toss the darned bit fields
> > from conn_rec, and certainly this one.
>
> I seriously disagree with this. bit fields are incredibly useful, and we
> should take advantage of them, not dump them.
>
FWIW, I am not a fan of bit fields. They are slower to check and all you get back is a bit of space
savings....maybe (if you cram multiple bit fields into an int).
Bill
Re: Multi-protocol patch, part 1
Posted by Chuck Murcko <ch...@topsail.org>.
On Tuesday, April 17, 2001, at 05:21 PM, Greg Stein wrote:
> On Tue, Apr 17, 2001 at 02:17:00PM -0400, Chuck Murcko wrote:
>
> Hmm. But we can only go hog-wild on a 2.1 with new features if it is
> separate from the 2.0 development (where we want to bugfix). That means
> two
> trees (whether that is two CVS repositories, or one rep with two
> branches).
> Bugger on maintenance since we will probably tweak 1.3 for a while, too.
>
> I'm not sure what the right approach is, but it would be good to start
> the
> list in the STATUS file, methinks.
>
Yeah. 2.0 still has to stabilize & perform enough to do a branch for
maintenance, and continue development on the HEAD.
Using STATUS to get started is +1 optimal right now.
>> ...
>> And I know that multiprotocol support has been on the list for 2.0
>> for a
>> little while now. We still have to talk about removing the proxy: hack.
>
> A "little while" ?? Heh. mod_echo was first checked in on August 23,
> 1999.
>
8^)
Chuck Murcko
Topsail Group
http://www.topsail.org/
Re: Multi-protocol patch, part 1
Posted by Greg Stein <gs...@lyra.org>.
On Tue, Apr 17, 2001 at 02:17:00PM -0400, Chuck Murcko wrote:
> David's got a valid point though: determinism and risk management for
> 2.0 release. And also about the schizoid nature of httpd going for
> release, stuff going in and stuff testing all at once.
Right. We can talk about "safe" patches all we want, but every one of them
could destabilize. Bugs exist because we miss things. There is nothing that
says we won't miss things in the patches.
> Maybe it's time to start actually putting things on the list for 2.1,
> like rethinking the scoreboard implementation or fully instrumenting the
> server. I mean, not everything's got time to make it into 2.0, so IMO we
> should start consciously slating big stuff for 2.1-5 timeframe.
Ah! YES. This is a great way to help release some pressure for feature
development.
Hmm. But we can only go hog-wild on a 2.1 with new features if it is
separate from the 2.0 development (where we want to bugfix). That means two
trees (whether that is two CVS repositories, or one rep with two branches).
Bugger on maintenance since we will probably tweak 1.3 for a while, too.
I'm not sure what the right approach is, but it would be good to start the
list in the STATUS file, methinks.
>...
> And I know that multiprotocol support has been on the list for 2.0 for a
> little while now. We still have to talk about removing the proxy: hack.
A "little while" ?? Heh. mod_echo was first checked in on August 23, 1999.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: Multi-protocol patch, part 1
Posted by David Reid <dr...@jetnet.co.uk>.
> David, I know some people (ISPs) who do around 6 months of
> installation testing before they make somethings operational.
> These are mostly done for core network features, but also
> for every service they use.
>
> I don't think they would mind to wait a month if they get
> what they want in 2.0. The mind earlier doing the operational
> testing twice.
I had a response written, but after reading it I decided it was continuing
the negativity of this thread so I'll save some bandwidth. We all do what
we want (as Ryan said). Have fun.
david
Re: Multi-protocol patch, part 1
Posted by Chuck Murcko <ch...@topsail.org>.
David's got a valid point though: determinism and risk management for
2.0 release. And also about the schizoid nature of httpd going for
release, stuff going in and stuff testing all at once.
Maybe it's time to start actually putting things on the list for 2.1,
like rethinking the scoreboard implementation or fully instrumenting the
server. I mean, not everything's got time to make it into 2.0, so IMO we
should start consciously slating big stuff for 2.1-5 timeframe.
Harry's right about official httpd users needing to run two test cycles
suckage (and some will argue that's not our problem). They'll have to
run a test cycle at each release, though.
Perhaps it will help everyone if we try to finalize the feature list for
2.0 soon, and get started on the 2.1-5 feature lists. You all know we're
gonna do that many, possibly on the first release. 8^)
And I know that multiprotocol support has been on the list for 2.0 for a
little while now. We still have to talk about removing the proxy: hack.
Chuck
On Tuesday, April 17, 2001, at 01:10 PM, Harrie Hazewinkel wrote:
> David Reid wrote:
>>
>> Have we agreed that the multi-protocol stuff that was outlined (and
>> this
>> patch is based on) is the correct way to go? I'm not convinced that
>> it is.
>> I REALLY dislike the fact that we appear to be pulling in 2 directions
>> at
>> the moment.
>>
>> One group is trying to get 2.0 bug fixed and ready for release, the
>> other
>> trying to destabilize it and insert new potential for bugs and
>> holes!! If
>> the multi-protocol stuff is a *MUST* have for a 2.0 release then we
>> should
>> have some real discussions and stop worrying about rolling "beta's".
>>
>> If it's not a must have then perhaps we should stop worrying about
>> this and
>> make it an issue for 2.1? Sorry to be blunt.
>
> David, I know some people (ISPs) who do around 6 months of
> installation testing before they make somethings operational.
> These are mostly done for core network features, but also
> for every service they use.
>
> I don't think they would mind to wait a month if they get
> what they want in 2.0. The mind earlier doing the operational
> testing twice.
>
> Harrie
> --
> phone: +39-3474932300
> http://www.lisanza.net/
>
Chuck Murcko
Topsail Group
http://www.topsail.org/
Re: Multi-protocol patch, part 1
Posted by Harrie Hazewinkel <ha...@covalent.net>.
David Reid wrote:
>
> Have we agreed that the multi-protocol stuff that was outlined (and this
> patch is based on) is the correct way to go? I'm not convinced that it is.
> I REALLY dislike the fact that we appear to be pulling in 2 directions at
> the moment.
>
> One group is trying to get 2.0 bug fixed and ready for release, the other
> trying to destabilize it and insert new potential for bugs and holes!! If
> the multi-protocol stuff is a *MUST* have for a 2.0 release then we should
> have some real discussions and stop worrying about rolling "beta's".
>
> If it's not a must have then perhaps we should stop worrying about this and
> make it an issue for 2.1? Sorry to be blunt.
David, I know some people (ISPs) who do around 6 months of
installation testing before they make somethings operational.
These are mostly done for core network features, but also
for every service they use.
I don't think they would mind to wait a month if they get
what they want in 2.0. The mind earlier doing the operational
testing twice.
Harrie
--
phone: +39-3474932300
http://www.lisanza.net/
Re: Multi-protocol patch, part 1
Posted by Greg Stein <gs...@lyra.org>.
On Tue, Apr 17, 2001 at 02:17:53PM -0400, Bill Stoddard wrote:
> > I am 100% against holding up a beta for this stuff. If we can keep the
> > patches incredibly small and reviewable, then there is no reason that they
> > have to destabilize the server.
>
> I agree that this work should not be allowed to hold up the beta (and
> eventual release) of Apache 2.0. One possible exception would be if the
> changes affect the external API in significant ways, which is not the case
> AFAIK. But I also agree that the changes can be phased in in small easily
> reviewable chunks. I am not against working incrementally to continuing to
> clean-up the parts of Apache 2.0 that hinder multiprotocol use.
This is my thinking, too. I've already described some of my concerns with
the approach for the multiprotocol stuff. As long as the patches are in the
non-controversial area, and are reviewable, then no problem.
When the patch arrives to insert a state_rec, then we'll start to see a
longer discussion about whether to commit them :-)
Also, note that the big mother patch that was on klomp is really just a
couple simple things, changed everywhere, and only a little bit of
multi-protocol work. Those simple things can go in, then we'd have a smaller
blob for Ryan's/Harrie's (Covalent's? :-)) ideas for multi-protocol.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: Multi-protocol patch, part 1
Posted by Bill Stoddard <bi...@wstoddard.com>.
>
> I am 100% against holding up a beta for this stuff. If we can keep the
> patches incredibly small and reviewable, then there is no reason that they
> have to destabilize the server.
>
I agree that this work should not be allowed to hold up the beta (and eventual release) of Apache
2.0. One possible exception would be if the changes affect the external API in significant ways,
which is not the case AFAIK. But I also agree that the changes can be phased in in small easily
reviewable chunks. I am not against working incrementally to continuing to clean-up the parts of
Apache 2.0 that hinder multiprotocol use.
> As for being blunt, isn't that why we all get along? If anybody on this
> list doesn't think they can speak their minds freely and bluntly, then
> things just don't work. :-)
>
> Ryan
>
> On Tue, 17 Apr 2001, David Reid wrote:
>
> > Have we agreed that the multi-protocol stuff that was outlined (and this
> > patch is based on) is the correct way to go? I'm not convinced that it is.
> > I REALLY dislike the fact that we appear to be pulling in 2 directions at
> > the moment.
> >
> > One group is trying to get 2.0 bug fixed and ready for release, the other
> > trying to destabilize it and insert new potential for bugs and holes!! If
> > the multi-protocol stuff is a *MUST* have for a 2.0 release then we should
> > have some real discussions and stop worrying about rolling "beta's".
> >
> > If it's not a must have then perhaps we should stop worrying about this and
> > make it an issue for 2.1? Sorry to be blunt.
> >
> > david
> > ----- Original Message -----
> > From: <rb...@covalent.net>
> > To: <ne...@apache.org>
> > Sent: Tuesday, April 17, 2001 4:34 PM
> > Subject: Re: Multi-protocol patch, part 1
> >
> >
> > >
> > > > It no workee. (timeout handling; see below)
> > >
> > > Argh! I fixed this once a long time ago, but now that fix is gone in both
> > > of my trees. Looking at it again. :-(
> > >
> > > > > +static int http_create_request(request_rec *r)
> > > > > +{
> > > > > + http_conn_rec *conn =
> > ap_get_module_config(r->connection->conn_config, &http_module);
> > > >
> > > > Nit. Please use "hconn" to distinguish this from the "old" conn. It's
> > hard
> > > > to read, when I keep thing conn_rec.
> > >
> > > done
> > >
> > > > > + conn = apr_palloc(r->pool, sizeof(*conn));
> > > >
> > > > Maybe use pcalloc here? Certainly, for future-proofing.
> > >
> > > done
> > >
> > > > Take a look at server/protocol.c, where this stuff came from. In between
> > the
> > > > two sets of the timeout, you'll see it goes to the socket to read the
> > > > request line. Basically, it sets the timeout to <something>, reads a
> > line,
> > > > then resets it to <something>.
> > > >
> > > > In the above code, it simply sets it a couple times and exits. Not very
> > > > effective :-)
> > >
> > > I'm fixing this now. I'll repost when I am done. This is part of the
> > > problem with splitting a large patch into a couple of small patches. Not
> > > that I am convinced it works correctly in the large patch, but I know I
> > > tackled this exact bug back when I first wrote the large patch. Expect
> > > the new patch later today.
> > >
> > > > >...
> > > > > --- modules/http/mod_core.h 2001/02/28 15:24:07 1.6
> > > > > +++ modules/http/mod_core.h 2001/04/17 03:53:23
> > > >
> > > > Hrm. It would be nice to rename this to mod_http.h before we get too
> > > > attached to the mod_core.h name. (totally unrelated to this patch, but
> > just
> > > > raising it while I'm thinking of it)
> > >
> > > I agree 100%
> > >
> > > > > +typedef struct http_conn_rec http_conn_rec;
> > > >
> > > > If you're introducing a new structure, then please namespace-protect it.
> > No
> > > > sense digging a deeper hole :-)
> > >
> > > I had kind of thought that just namespace protecting it with http_ was
> > > okay. However, now that you mention it, I should use ap_http_, so I have
> > > done that now.
> > >
> > > > However, please note that we can/should simply toss the darned bit
> > fields
> > > > from conn_rec, and certainly this one.
> > >
> > > I seriously disagree with this. bit fields are incredibly useful, and we
> > > should take advantage of them, not dump them.
> > >
> > > > > #if APR_HAVE_UNISTD_H
> > > > > #include <unistd.h>
> > > > > @@ -527,13 +528,19 @@
> > > > > }
> > > > > static const char *log_connection_status(request_rec *r, char *a)
> > > > > {
> > > > > +#ifdef AP_HTTP_ENABLED
> > > > > + http_conn_rec *hconn =
> > ap_get_module_config(r->connection->conn_config,
> > > > > + &http_module);
> > > > > +#endif
> > > >
> > > > Oof! It would be nice to keep the http_conn_rec private. This kinda
> > sucks.
> > >
> > > I disagree, this REALLY sucks. :-) I think I know how to fix it, but it
> > > is a relatively large change, so I really don't want to make it here.
> > > What I believe we should basically do, is register an optional function
> > > that takes a format specifier and a function to call. Basically, the same
> > > thing we do for mod_include and mod_cgi. This is a much more extensible
> > > option.
> > >
> > > > > r->connection = conn;
> > > > > r->server = conn->base_server;
> > > > >
> > > > > - conn->keptalive = conn->keepalive == 1;
> > > >
> > > > Okay... about conn->keptalive.
> > >
> > > finally. :-)
> > >
> > > > conn_rec, but used *only* in this function.
> > > >
> > > > Rather than shift it from conn_rec to http_conn_rec, let's just switch
> > it to
> > > > a local variable here(!)
> > >
> > > Done!!!! Great Catch Greg!
> > >
> > > Ryan
> > >
> > >
> > ____________________________________________________________________________
> > ___
> > > Ryan Bloom rbb@apache.org
> > > 406 29th St.
> > > San Francisco, CA 94131
> > > --------------------------------------------------------------------------
> > -----
> > >
> >
> >
>
>
> _______________________________________________________________________________
> Ryan Bloom rbb@apache.org
> 406 29th St.
> San Francisco, CA 94131
> -------------------------------------------------------------------------------
>
Re: Multi-protocol patch, part 1
Posted by rb...@covalent.net.
I don't think we are pulling in two directions. I think that increasing
the modularity of the server is a good thing for stability and
maintainability.
I am trying very hard to make the patches very small and thus to not
impact the workings of the server too much.
We have said many times that we all work on what is important to us. We
have also said that this is a multi-protocol server. If 2.0 is going to
be multi-protocol, then we need to have the discussion. If it isn't, then
we need to stop saying it is.
I am 100% against holding up a beta for this stuff. If we can keep the
patches incredibly small and reviewable, then there is no reason that they
have to destabilize the server.
As for being blunt, isn't that why we all get along? If anybody on this
list doesn't think they can speak their minds freely and bluntly, then
things just don't work. :-)
Ryan
On Tue, 17 Apr 2001, David Reid wrote:
> Have we agreed that the multi-protocol stuff that was outlined (and this
> patch is based on) is the correct way to go? I'm not convinced that it is.
> I REALLY dislike the fact that we appear to be pulling in 2 directions at
> the moment.
>
> One group is trying to get 2.0 bug fixed and ready for release, the other
> trying to destabilize it and insert new potential for bugs and holes!! If
> the multi-protocol stuff is a *MUST* have for a 2.0 release then we should
> have some real discussions and stop worrying about rolling "beta's".
>
> If it's not a must have then perhaps we should stop worrying about this and
> make it an issue for 2.1? Sorry to be blunt.
>
> david
> ----- Original Message -----
> From: <rb...@covalent.net>
> To: <ne...@apache.org>
> Sent: Tuesday, April 17, 2001 4:34 PM
> Subject: Re: Multi-protocol patch, part 1
>
>
> >
> > > It no workee. (timeout handling; see below)
> >
> > Argh! I fixed this once a long time ago, but now that fix is gone in both
> > of my trees. Looking at it again. :-(
> >
> > > > +static int http_create_request(request_rec *r)
> > > > +{
> > > > + http_conn_rec *conn =
> ap_get_module_config(r->connection->conn_config, &http_module);
> > >
> > > Nit. Please use "hconn" to distinguish this from the "old" conn. It's
> hard
> > > to read, when I keep thing conn_rec.
> >
> > done
> >
> > > > + conn = apr_palloc(r->pool, sizeof(*conn));
> > >
> > > Maybe use pcalloc here? Certainly, for future-proofing.
> >
> > done
> >
> > > Take a look at server/protocol.c, where this stuff came from. In between
> the
> > > two sets of the timeout, you'll see it goes to the socket to read the
> > > request line. Basically, it sets the timeout to <something>, reads a
> line,
> > > then resets it to <something>.
> > >
> > > In the above code, it simply sets it a couple times and exits. Not very
> > > effective :-)
> >
> > I'm fixing this now. I'll repost when I am done. This is part of the
> > problem with splitting a large patch into a couple of small patches. Not
> > that I am convinced it works correctly in the large patch, but I know I
> > tackled this exact bug back when I first wrote the large patch. Expect
> > the new patch later today.
> >
> > > >...
> > > > --- modules/http/mod_core.h 2001/02/28 15:24:07 1.6
> > > > +++ modules/http/mod_core.h 2001/04/17 03:53:23
> > >
> > > Hrm. It would be nice to rename this to mod_http.h before we get too
> > > attached to the mod_core.h name. (totally unrelated to this patch, but
> just
> > > raising it while I'm thinking of it)
> >
> > I agree 100%
> >
> > > > +typedef struct http_conn_rec http_conn_rec;
> > >
> > > If you're introducing a new structure, then please namespace-protect it.
> No
> > > sense digging a deeper hole :-)
> >
> > I had kind of thought that just namespace protecting it with http_ was
> > okay. However, now that you mention it, I should use ap_http_, so I have
> > done that now.
> >
> > > However, please note that we can/should simply toss the darned bit
> fields
> > > from conn_rec, and certainly this one.
> >
> > I seriously disagree with this. bit fields are incredibly useful, and we
> > should take advantage of them, not dump them.
> >
> > > > #if APR_HAVE_UNISTD_H
> > > > #include <unistd.h>
> > > > @@ -527,13 +528,19 @@
> > > > }
> > > > static const char *log_connection_status(request_rec *r, char *a)
> > > > {
> > > > +#ifdef AP_HTTP_ENABLED
> > > > + http_conn_rec *hconn =
> ap_get_module_config(r->connection->conn_config,
> > > > + &http_module);
> > > > +#endif
> > >
> > > Oof! It would be nice to keep the http_conn_rec private. This kinda
> sucks.
> >
> > I disagree, this REALLY sucks. :-) I think I know how to fix it, but it
> > is a relatively large change, so I really don't want to make it here.
> > What I believe we should basically do, is register an optional function
> > that takes a format specifier and a function to call. Basically, the same
> > thing we do for mod_include and mod_cgi. This is a much more extensible
> > option.
> >
> > > > r->connection = conn;
> > > > r->server = conn->base_server;
> > > >
> > > > - conn->keptalive = conn->keepalive == 1;
> > >
> > > Okay... about conn->keptalive.
> >
> > finally. :-)
> >
> > > conn_rec, but used *only* in this function.
> > >
> > > Rather than shift it from conn_rec to http_conn_rec, let's just switch
> it to
> > > a local variable here(!)
> >
> > Done!!!! Great Catch Greg!
> >
> > Ryan
> >
> >
> ____________________________________________________________________________
> ___
> > Ryan Bloom rbb@apache.org
> > 406 29th St.
> > San Francisco, CA 94131
> > --------------------------------------------------------------------------
> -----
> >
>
>
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: Multi-protocol patch, part 1
Posted by David Reid <dr...@jetnet.co.uk>.
Have we agreed that the multi-protocol stuff that was outlined (and this
patch is based on) is the correct way to go? I'm not convinced that it is.
I REALLY dislike the fact that we appear to be pulling in 2 directions at
the moment.
One group is trying to get 2.0 bug fixed and ready for release, the other
trying to destabilize it and insert new potential for bugs and holes!! If
the multi-protocol stuff is a *MUST* have for a 2.0 release then we should
have some real discussions and stop worrying about rolling "beta's".
If it's not a must have then perhaps we should stop worrying about this and
make it an issue for 2.1? Sorry to be blunt.
david
----- Original Message -----
From: <rb...@covalent.net>
To: <ne...@apache.org>
Sent: Tuesday, April 17, 2001 4:34 PM
Subject: Re: Multi-protocol patch, part 1
>
> > It no workee. (timeout handling; see below)
>
> Argh! I fixed this once a long time ago, but now that fix is gone in both
> of my trees. Looking at it again. :-(
>
> > > +static int http_create_request(request_rec *r)
> > > +{
> > > + http_conn_rec *conn =
ap_get_module_config(r->connection->conn_config, &http_module);
> >
> > Nit. Please use "hconn" to distinguish this from the "old" conn. It's
hard
> > to read, when I keep thing conn_rec.
>
> done
>
> > > + conn = apr_palloc(r->pool, sizeof(*conn));
> >
> > Maybe use pcalloc here? Certainly, for future-proofing.
>
> done
>
> > Take a look at server/protocol.c, where this stuff came from. In between
the
> > two sets of the timeout, you'll see it goes to the socket to read the
> > request line. Basically, it sets the timeout to <something>, reads a
line,
> > then resets it to <something>.
> >
> > In the above code, it simply sets it a couple times and exits. Not very
> > effective :-)
>
> I'm fixing this now. I'll repost when I am done. This is part of the
> problem with splitting a large patch into a couple of small patches. Not
> that I am convinced it works correctly in the large patch, but I know I
> tackled this exact bug back when I first wrote the large patch. Expect
> the new patch later today.
>
> > >...
> > > --- modules/http/mod_core.h 2001/02/28 15:24:07 1.6
> > > +++ modules/http/mod_core.h 2001/04/17 03:53:23
> >
> > Hrm. It would be nice to rename this to mod_http.h before we get too
> > attached to the mod_core.h name. (totally unrelated to this patch, but
just
> > raising it while I'm thinking of it)
>
> I agree 100%
>
> > > +typedef struct http_conn_rec http_conn_rec;
> >
> > If you're introducing a new structure, then please namespace-protect it.
No
> > sense digging a deeper hole :-)
>
> I had kind of thought that just namespace protecting it with http_ was
> okay. However, now that you mention it, I should use ap_http_, so I have
> done that now.
>
> > However, please note that we can/should simply toss the darned bit
fields
> > from conn_rec, and certainly this one.
>
> I seriously disagree with this. bit fields are incredibly useful, and we
> should take advantage of them, not dump them.
>
> > > #if APR_HAVE_UNISTD_H
> > > #include <unistd.h>
> > > @@ -527,13 +528,19 @@
> > > }
> > > static const char *log_connection_status(request_rec *r, char *a)
> > > {
> > > +#ifdef AP_HTTP_ENABLED
> > > + http_conn_rec *hconn =
ap_get_module_config(r->connection->conn_config,
> > > + &http_module);
> > > +#endif
> >
> > Oof! It would be nice to keep the http_conn_rec private. This kinda
sucks.
>
> I disagree, this REALLY sucks. :-) I think I know how to fix it, but it
> is a relatively large change, so I really don't want to make it here.
> What I believe we should basically do, is register an optional function
> that takes a format specifier and a function to call. Basically, the same
> thing we do for mod_include and mod_cgi. This is a much more extensible
> option.
>
> > > r->connection = conn;
> > > r->server = conn->base_server;
> > >
> > > - conn->keptalive = conn->keepalive == 1;
> >
> > Okay... about conn->keptalive.
>
> finally. :-)
>
> > conn_rec, but used *only* in this function.
> >
> > Rather than shift it from conn_rec to http_conn_rec, let's just switch
it to
> > a local variable here(!)
>
> Done!!!! Great Catch Greg!
>
> Ryan
>
>
____________________________________________________________________________
___
> Ryan Bloom rbb@apache.org
> 406 29th St.
> San Francisco, CA 94131
> --------------------------------------------------------------------------
-----
>
Re: Multi-protocol patch, part 1
Posted by rb...@covalent.net.
> It no workee. (timeout handling; see below)
Argh! I fixed this once a long time ago, but now that fix is gone in both
of my trees. Looking at it again. :-(
> > +static int http_create_request(request_rec *r)
> > +{
> > + http_conn_rec *conn = ap_get_module_config(r->connection->conn_config, &http_module);
>
> Nit. Please use "hconn" to distinguish this from the "old" conn. It's hard
> to read, when I keep thing conn_rec.
done
> > + conn = apr_palloc(r->pool, sizeof(*conn));
>
> Maybe use pcalloc here? Certainly, for future-proofing.
done
> Take a look at server/protocol.c, where this stuff came from. In between the
> two sets of the timeout, you'll see it goes to the socket to read the
> request line. Basically, it sets the timeout to <something>, reads a line,
> then resets it to <something>.
>
> In the above code, it simply sets it a couple times and exits. Not very
> effective :-)
I'm fixing this now. I'll repost when I am done. This is part of the
problem with splitting a large patch into a couple of small patches. Not
that I am convinced it works correctly in the large patch, but I know I
tackled this exact bug back when I first wrote the large patch. Expect
the new patch later today.
> >...
> > --- modules/http/mod_core.h 2001/02/28 15:24:07 1.6
> > +++ modules/http/mod_core.h 2001/04/17 03:53:23
>
> Hrm. It would be nice to rename this to mod_http.h before we get too
> attached to the mod_core.h name. (totally unrelated to this patch, but just
> raising it while I'm thinking of it)
I agree 100%
> > +typedef struct http_conn_rec http_conn_rec;
>
> If you're introducing a new structure, then please namespace-protect it. No
> sense digging a deeper hole :-)
I had kind of thought that just namespace protecting it with http_ was
okay. However, now that you mention it, I should use ap_http_, so I have
done that now.
> However, please note that we can/should simply toss the darned bit fields
> from conn_rec, and certainly this one.
I seriously disagree with this. bit fields are incredibly useful, and we
should take advantage of them, not dump them.
> > #if APR_HAVE_UNISTD_H
> > #include <unistd.h>
> > @@ -527,13 +528,19 @@
> > }
> > static const char *log_connection_status(request_rec *r, char *a)
> > {
> > +#ifdef AP_HTTP_ENABLED
> > + http_conn_rec *hconn = ap_get_module_config(r->connection->conn_config,
> > + &http_module);
> > +#endif
>
> Oof! It would be nice to keep the http_conn_rec private. This kinda sucks.
I disagree, this REALLY sucks. :-) I think I know how to fix it, but it
is a relatively large change, so I really don't want to make it here.
What I believe we should basically do, is register an optional function
that takes a format specifier and a function to call. Basically, the same
thing we do for mod_include and mod_cgi. This is a much more extensible
option.
> > r->connection = conn;
> > r->server = conn->base_server;
> >
> > - conn->keptalive = conn->keepalive == 1;
>
> Okay... about conn->keptalive.
finally. :-)
> conn_rec, but used *only* in this function.
>
> Rather than shift it from conn_rec to http_conn_rec, let's just switch it to
> a local variable here(!)
Done!!!! Great Catch Greg!
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: Multi-protocol patch, part 1
Posted by Greg Stein <gs...@lyra.org>.
On Mon, Apr 16, 2001 at 09:07:02PM -0700, rbb@covalent.net wrote:
>...
> What this post basically does, is it breaks part of the conn_rec out and
> into an HTTP specific structure. I am not saying that this is everything
> that can be pulled out of the conn_rec, but it is a good first step.
>
> Comments?
It no workee. (timeout handling; see below)
>...
> --- modules/http/http_core.c 2001/04/11 23:37:16 1.270
> +++ modules/http/http_core.c 2001/04/17 03:53:22
> @@ -295,6 +295,35 @@
> return OK;
> }
>
> +static int http_create_request(request_rec *r)
> +{
> + http_conn_rec *conn = ap_get_module_config(r->connection->conn_config, &http_module);
Nit. Please use "hconn" to distinguish this from the "old" conn. It's hard
to read, when I keep thing conn_rec.
> + conn = apr_palloc(r->pool, sizeof(*conn));
Maybe use pcalloc here? Certainly, for future-proofing.
> + ap_set_module_config(r->connection->conn_config, &http_module, conn);
> +
> + if (!r->main && !(r->prev || r->next)) {
> + conn->keptalive = r->connection->keepalive == 1;
> + r->connection->keepalive = 0;
> +
> + apr_setsocketopt(r->connection->client_socket, APR_SO_TIMEOUT,
> + (int)(conn->keptalive
> + ? r->server->keep_alive_timeout * APR_USEC_PER_SEC
> + : r->server->timeout * APR_USEC_PER_SEC));
> +
> + if (conn->keptalive) {
> + apr_setsocketopt(r->connection->client_socket,
> + APR_SO_TIMEOUT,
> + (int)(r->server->timeout * APR_USEC_PER_SEC));
> + }
Take a look at server/protocol.c, where this stuff came from. In between the
two sets of the timeout, you'll see it goes to the socket to read the
request line. Basically, it sets the timeout to <something>, reads a line,
then resets it to <something>.
In the above code, it simply sets it a couple times and exits. Not very
effective :-)
> + conn->keptalive = 0;
See note below about this variable.
>...
> --- modules/http/mod_core.h 2001/02/28 15:24:07 1.6
> +++ modules/http/mod_core.h 2001/04/17 03:53:23
Hrm. It would be nice to rename this to mod_http.h before we get too
attached to the mod_core.h name. (totally unrelated to this patch, but just
raising it while I'm thinking of it)
> @@ -70,6 +70,15 @@
> * @package mod_core private header file
> */
>
> +typedef struct http_conn_rec http_conn_rec;
If you're introducing a new structure, then please namespace-protect it. No
sense digging a deeper hole :-)
> +struct http_conn_rec {
> + /** Did we use HTTP Keep-Alive? */
> + unsigned keptalive:1;
See below about this variable.
However, please note that we can/should simply toss the darned bit fields
from conn_rec, and certainly this one.
>...
> --- modules/loggers/mod_log_config.c 2001/03/09 20:30:32 1.52
> +++ modules/loggers/mod_log_config.c 2001/04/17 03:53:23
> @@ -191,6 +191,7 @@
> #include "http_core.h" /* For REMOTE_NAME */
> #include "http_log.h"
> #include "http_protocol.h"
> +#include "mod_core.h"
>
> #if APR_HAVE_UNISTD_H
> #include <unistd.h>
> @@ -527,13 +528,19 @@
> }
> static const char *log_connection_status(request_rec *r, char *a)
> {
> +#ifdef AP_HTTP_ENABLED
> + http_conn_rec *hconn = ap_get_module_config(r->connection->conn_config,
> + &http_module);
> +#endif
Oof! It would be nice to keep the http_conn_rec private. This kinda sucks.
Maybe for another day, though.
>...
> --- server/protocol.c 2001/04/11 23:37:16 1.12
> +++ server/protocol.c 2001/04/17 03:53:26
> @@ -556,9 +556,6 @@
> r->connection = conn;
> r->server = conn->base_server;
>
> - conn->keptalive = conn->keepalive == 1;
Okay... about conn->keptalive.
I just searched the whole httpd-2.0 repository. This value is defined in the
conn_rec, but used *only* in this function.
Rather than shift it from conn_rec to http_conn_rec, let's just switch it to
a local variable here(!)
> - conn->keepalive = 0;
> -
> r->user = NULL;
> r->ap_auth_type = NULL;
>
> @@ -584,11 +581,6 @@
> r->output_filters = conn->output_filters;
> r->input_filters = conn->input_filters;
>
> - apr_setsocketopt(conn->client_socket, APR_SO_TIMEOUT,
> - (int)(conn->keptalive
> - ? r->server->keep_alive_timeout * APR_USEC_PER_SEC
> - : r->server->timeout * APR_USEC_PER_SEC));
> -
> ap_add_output_filter("BYTERANGE", NULL, r, r->connection);
> ap_add_output_filter("CONTENT_LENGTH", NULL, r, r->connection);
> ap_add_output_filter("HTTP_HEADER", NULL, r, r->connection);
> @@ -604,10 +596,6 @@
> }
> return NULL;
> }
> - if (r->connection->keptalive) {
This is a longer way of saying conn->keptalive. But that is a long way of
referring the local variable "keptalive".
> - apr_setsocketopt(r->connection->client_socket, APR_SO_TIMEOUT,
> - (int)(r->server->timeout * APR_USEC_PER_SEC));
This little block of code says, "it we set the timeout to something other
than server->timeout (above), then set it back."
I'm not sure that we can extract this to mod_http. Maybe conn->keepalives,
but I think this patch needs a bit of rethink, regarding this timeout
handling code.
> - }
> if (!r->assbackwards) {
> get_mime_headers(r);
> if (r->status != HTTP_REQUEST_TIME_OUT) {
> @@ -645,8 +633,6 @@
>
> /* we may have switched to another server */
> r->per_dir_config = r->server->lookup_defaults;
> -
> - conn->keptalive = 0; /* We now have a request to play with */
Local var again.
There may be some other review comments, but I concentrated mostly trying to
describe the above issue w.r.t timeouts and stuff. Happy to review an
updated patch, of course!
Cheers,
-g
--
Greg Stein, http://www.lyra.org/