Re: [Patch] Keep Alive not workwing with mod_proxy (PR38602)
On Feb 13, 2006, at 6:25 PM, Ruediger Pluem wrote: Then we should either find out or adjust it to the behaviour that we think is correct as the current behaviour doesn't seem to be. This looks to be an almost direct port from mod_jk, but I agree that the current behavior is quite strange :) So instead of worrying about why it is the way it is, I agree that we just fix what's there. We have a framework for true pooling, so let's just use that.;) Does the patched version pass the test framework? Have not checked so far. I did not manage to get the test framework running on my box so far. Can someone who has it running give it a try? That would be very nice. Although it's not really documented anyplace, it really is good practice for people who submit large changes to run them through the test framework first; it might be good to take same time and try to get it up and running, since it really does help track some things down... In the meantime, if you can send the latest patch, I'll test it here.
Re: [Patch] Keep Alive not workwing with mod_proxy (PR38602)
On Feb 13, 2006, at 1:28 PM, William A. Rowe, Jr. wrote: Ruediger Pluem wrote: Currently I work on PR 38602 (http://issues.apache.org/bugzilla/ show_bug.cgi?id=38602). First of all the reporter is correct that we do not sent the Connection: Keep-Alive header on our HTTP/1.1 keep-alive connections to the backend. But this is only the small part of the problem since 8.1.2 of the RFC says: A significant difference between HTTP/1.1 and earlier versions of HTTP is that persistent connections are the default behavior of any HTTP connection. That is, unless otherwise indicated, the client SHOULD assume that the server will maintain a persistent connection, The real problem is that we've never paid attention to the backend server. If speaking to a backend http/1.0 server, we can try connection: keep-alive if the server pays attention to it. That header is invalid for http/1.1 backends, and we should choose connection: close where appropriate. To a backend http/1.0 server, connection: close is meaningless (and wrong). IIRC, http/1.0 lacks any Connection header at all.
Re: [Patch] Keep Alive not workwing with mod_proxy (PR38602)
Jim Jagielski wrote: To a backend http/1.0 server, connection: close is meaningless (and wrong). IIRC, http/1.0 lacks any Connection header at all. connection: keep-alive was a transitional http/1.0 behavior.
Re: [Patch] Keep Alive not workwing with mod_proxy (PR38602)
William A. Rowe, Jr. wrote: Jim Jagielski wrote: To a backend http/1.0 server, connection: close is meaningless (and wrong). IIRC, http/1.0 lacks any Connection header at all. connection: keep-alive was a transitional http/1.0 behavior. Yes, but not formal 1.0. (rfc1945) -- === Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/ If you can dodge a wrench, you can dodge a ball.
Re: [Patch] Keep Alive not workwing with mod_proxy (PR38602)
On 02/14/2006 01:51 PM, Jim Jagielski wrote:
since it really does help track some things down... In the
meantime, if you can send the latest patch, I'll test
it here.
Please find attached
Changes to the previous one:
1. Diff against r377821.
2. I removed the !backend check also.
I am curious about the test results.
Regards
Rüdiger
Index: modules/proxy/proxy_util.c
===
--- modules/proxy/proxy_util.c (Revision 377821)
+++ modules/proxy/proxy_util.c (Arbeitskopie)
@@ -1868,16 +1868,6 @@
conn-hostname = apr_pstrdup(conn-pool, uri-hostname);
conn-port = uri-port;
}
-}
-/*
- * TODO: add address cache for generic forward proxies.
- * At least level 0 - compare with previous hostname:port
- */
-if (r-proxyreq == PROXYREQ_PROXY || r-proxyreq == PROXYREQ_REVERSE ||
-!worker-is_address_reusable) {
-/*
- * TODO: Check if the connection can be reused
- */
if (conn-connection) {
if (conn-sock) {
apr_socket_close(conn-sock);
Index: modules/proxy/mod_proxy_http.c
===
--- modules/proxy/mod_proxy_http.c (Revision 377821)
+++ modules/proxy/mod_proxy_http.c (Arbeitskopie)
@@ -981,9 +981,18 @@
/* Yes I hate gotos. This is the subrequest shortcut */
skip_body:
-/* Handle Connection: header */
-if (!force10 p_conn-close) {
-buf = apr_pstrdup(p, Connection: close CRLF);
+/*
+ * Handle Connection: header if we do HTTP/1.1 request:
+ * If we plan to close the backend connection sent Connection: close
+ * otherwise sent Connection: Keep-Alive.
+ */
+if (!force10) {
+if (p_conn-close) {
+buf = apr_pstrdup(p, Connection: close CRLF);
+}
+else {
+buf = apr_pstrdup(p, Connection: Keep-Alive CRLF);
+}
ap_xlate_proto_to_ascii(buf, strlen(buf));
e = apr_bucket_pool_create(buf, strlen(buf), p, c-bucket_alloc);
APR_BRIGADE_INSERT_TAIL(header_brigade, e);
@@ -1510,12 +1519,6 @@
/* found the last brigade? */
if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(bb))) {
-/* if this is the last brigade, cleanup the
- * backend connection first to prevent the
- * backend server from hanging around waiting
- * for a slow client to eat these bytes
- */
-backend-close = 1;
/* signal that we must leave */
finish = TRUE;
}
@@ -1584,18 +1587,7 @@
apr_status_t ap_proxy_http_cleanup(const char *scheme, request_rec *r,
proxy_conn_rec *backend)
{
-/* If there are no KeepAlives, or if the connection has been signalled
- * to close, close the socket and clean up
- */
-
-/* if the connection is HTTP/1.1, or Connection: close,
- * we close the socket, otherwise we leave it open for KeepAlive support
- */
-if (backend-close || (r-proto_num HTTP_VERSION(1,1))) {
-backend-close_on_recycle = 1;
-ap_set_module_config(r-connection-conn_config, proxy_http_module,
NULL);
-ap_proxy_release_connection(scheme, backend, r-server);
-}
+ap_proxy_release_connection(scheme, backend, r-server);
return OK;
}
@@ -1673,26 +1665,13 @@
proxy: HTTP: serving URL %s, url);
-/* only use stored info for top-level pages. Sub requests don't share
- * in keepalives
- */
-if (!r-main) {
-backend = (proxy_conn_rec *) ap_get_module_config(c-conn_config,
- proxy_http_module);
-}
/* create space for state information */
-if (!backend) {
-if ((status = ap_proxy_acquire_connection(proxy_function, backend,
- worker, r-server)) != OK)
-goto cleanup;
+if ((status = ap_proxy_acquire_connection(proxy_function, backend,
+ worker, r-server)) != OK)
+goto cleanup;
-if (!r-main) {
-ap_set_module_config(c-conn_config, proxy_http_module, backend);
-}
-}
backend-is_ssl = is_ssl;
-backend-close_on_recycle = 1;
/* Step One: Determine Who To Connect To */
if ((status = ap_proxy_determine_connection(p, r, conf, worker, backend,
@@ -1732,10 +1711,8 @@
cleanup:
if (backend) {
-if (status != OK) {
+if (status != OK)
backend-close = 1;
-backend-close_on_recycle = 1;
-}
ap_proxy_http_cleanup(proxy_function, r, backend);
}
return status;
Re: [Patch] Keep Alive not workwing with mod_proxy (PR38602)
I'll test tomorrow morning... Heading out early today for Valentine's Day :) Ruediger Pluem wrote: This is a multi-part message in MIME format. --020401000706000306050001 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On 02/14/2006 01:51 PM, Jim Jagielski wrote: since it really does help track some things down... In the meantime, if you can send the latest patch, I'll test it here. Please find attached Changes to the previous one: 1. Diff against r377821. 2. I removed the !backend check also. I am curious about the test results. Regards Rüdiger --020401000706000306050001 Content-Type: text/plain; name=38602.diff Content-Transfer-Encoding: base64 Content-Disposition: inline; filename=38602.diff SW5kZXg6IG1vZHVsZXMvcHJveHkvcHJveHlfdXRpbC5jCj09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIG1v ZHVsZXMvcHJveHkvcHJveHlfdXRpbC5jCShSZXZpc2lvbiAzNzc4MjEpCisrKyBtb2R1bGVz L3Byb3h5L3Byb3h5X3V0aWwuYwkoQXJiZWl0c2tvcGllKQpAQCAtMTg2OCwxNiArMTg2OCw2 IEBACiAgICAgICAgICAgICBjb25uLT5ob3N0bmFtZSA9IGFwcl9wc3RyZHVwKGNvbm4tPnBv b2wsIHVyaS0+aG9zdG5hbWUpOwogICAgICAgICAgICAgY29ubi0+cG9ydCA9IHVyaS0+cG9y dDsKICAgICAgICAgfQotICAgIH0KLSAgICAvKgotICAgICAqIFRPRE86IGFkZCBhZGRyZXNz IGNhY2hlIGZvciBnZW5lcmljIGZvcndhcmQgcHJveGllcy4KLSAgICAgKiBBdCBsZWFzdCBs ZXZlbCAwIC0+IGNvbXBhcmUgd2l0aCBwcmV2aW91cyBob3N0bmFtZTpwb3J0Ci0gICAgICov Ci0gICAgaWYgKHItPnByb3h5cmVxID09IFBST1hZUkVRX1BST1hZIHx8IHItPnByb3h5cmVx ID09IFBST1hZUkVRX1JFVkVSU0UgfHwKLSAgICAgICAgIXdvcmtlci0+aXNfYWRkcmVzc19y ZXVzYWJsZSkgewotICAgICAgICAvKgotICAgICAgICAgKiBUT0RPOiBDaGVjayBpZiB0aGUg Y29ubmVjdGlvbiBjYW4gYmUgcmV1c2VkCi0gICAgICAgICAqLwogICAgICAgICBpZiAoY29u bi0+Y29ubmVjdGlvbikgewogICAgICAgICAgICAgaWYgKGNvbm4tPnNvY2spIHsKICAgICAg ICAgICAgICAgICBhcHJfc29ja2V0X2Nsb3NlKGNvbm4tPnNvY2spOwpJbmRleDogbW9kdWxl cy9wcm94eS9tb2RfcHJveHlfaHR0cC5jCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIG1vZHVsZXMvcHJv eHkvbW9kX3Byb3h5X2h0dHAuYwkoUmV2aXNpb24gMzc3ODIxKQorKysgbW9kdWxlcy9wcm94 eS9tb2RfcHJveHlfaHR0cC5jCShBcmJlaXRza29waWUpCkBAIC05ODEsOSArOTgxLDE4IEBA CiAKIC8qIFllcyBJIGhhdGUgZ290b3MuICBUaGlzIGlzIHRoZSBzdWJyZXF1ZXN0IHNob3J0 Y3V0ICovCiBza2lwX2JvZHk6Ci0gICAgLyogSGFuZGxlIENvbm5lY3Rpb246IGhlYWRlciAq LwotICAgIGlmICghZm9yY2UxMCAmJiBwX2Nvbm4tPmNsb3NlKSB7Ci0gICAgICAgIGJ1ZiA9 IGFwcl9wc3RyZHVwKHAsICJDb25uZWN0aW9uOiBjbG9zZSIgQ1JMRik7CisgICAgLyoKKyAg ICAgKiBIYW5kbGUgQ29ubmVjdGlvbjogaGVhZGVyIGlmIHdlIGRvIEhUVFAvMS4xIHJlcXVl c3Q6CisgICAgICogSWYgd2UgcGxhbiB0byBjbG9zZSB0aGUgYmFja2VuZCBjb25uZWN0aW9u IHNlbnQgQ29ubmVjdGlvbjogY2xvc2UKKyAgICAgKiBvdGhlcndpc2Ugc2VudCBDb25uZWN0 aW9uOiBLZWVwLUFsaXZlLgorICAgICAqLworICAgIGlmICghZm9yY2UxMCkgeworICAgICAg ICBpZiAocF9jb25uLT5jbG9zZSkgeworICAgICAgICAgICAgYnVmID0gYXByX3BzdHJkdXAo cCwgIkNvbm5lY3Rpb246IGNsb3NlIiBDUkxGKTsKKyAgICAgICAgfQorICAgICAgICBlbHNl IHsKKyAgICAgICAgICAgIGJ1ZiA9IGFwcl9wc3RyZHVwKHAsICJDb25uZWN0aW9uOiBLZWVw LUFsaXZlIiBDUkxGKTsKKyAgICAgICAgfQogICAgICAgICBhcF94bGF0ZV9wcm90b190b19h c2NpaShidWYsIHN0cmxlbihidWYpKTsKICAgICAgICAgZSA9IGFwcl9idWNrZXRfcG9vbF9j cmVhdGUoYnVmLCBzdHJsZW4oYnVmKSwgcCwgYy0+YnVja2V0X2FsbG9jKTsKICAgICAgICAg QVBSX0JSSUdBREVfSU5TRVJUX1RBSUwoaGVhZGVyX2JyaWdhZGUsIGUpOwpAQCAtMTUxMCwx MiArMTUxOSw2IEBACiAKICAgICAgICAgICAgICAgICAgICAgLyogZm91bmQgdGhlIGxhc3Qg YnJpZ2FkZT8gKi8KICAgICAgICAgICAgICAgICAgICAgaWYgKEFQUl9CVUNLRVRfSVNfRU9T KEFQUl9CUklHQURFX0xBU1QoYmIpKSkgewotICAgICAgICAgICAgICAgICAgICAgICAgLyog aWYgdGhpcyBpcyB0aGUgbGFzdCBicmlnYWRlLCBjbGVhbnVwIHRoZQotICAgICAgICAgICAg ICAgICAgICAgICAgICogYmFja2VuZCBjb25uZWN0aW9uIGZpcnN0IHRvIHByZXZlbnQgdGhl Ci0gICAgICAgICAgICAgICAgICAgICAgICAgKiBiYWNrZW5kIHNlcnZlciBmcm9tIGhhbmdp bmcgYXJvdW5kIHdhaXRpbmcKLSAgICAgICAgICAgICAgICAgICAgICAgICAqIGZvciBhIHNs b3cgY2xpZW50IHRvIGVhdCB0aGVzZSBieXRlcwotICAgICAgICAgICAgICAgICAgICAgICAg ICovCi0gICAgICAgICAgICAgICAgICAgICAgICBiYWNrZW5kLT5jbG9zZSA9IDE7CiAgICAg ICAgICAgICAgICAgICAgICAgICAvKiBzaWduYWwgdGhhdCB3ZSBtdXN0IGxlYXZlICovCiAg ICAgICAgICAgICAgICAgICAgICAgICBmaW5pc2ggPSBUUlVFOwogICAgICAgICAgICAgICAg ICAgICB9CkBAIC0xNTg0LDE4ICsxNTg3LDcgQEAKIGFwcl9zdGF0dXNfdCBhcF9wcm94eV9o dHRwX2NsZWFudXAoY29uc3QgY2hhciAqc2NoZW1lLCByZXF1ZXN0X3JlYyAqciwKICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgcHJveHlfY29ubl9yZWMgKmJhY2tlbmQp CiB7Ci0gICAgLyogSWYgdGhlcmUgYXJlIG5vIEtlZXBBbGl2ZXMsIG9yIGlmIHRoZSBjb25u ZWN0aW9uIGhhcyBiZWVuIHNpZ25hbGxlZAotICAgICAqIHRvIGNsb3NlLCBjbG9zZSB0aGUg c29ja2V0IGFuZCBjbGVhbiB1cAotICAgICAqLwotCi0gICAgLyogaWYgdGhlIGNvbm5lY3Rp b24gaXMgPCBIVFRQLzEuMSwgb3IgQ29ubmVjdGlvbjogY2xvc2UsCi0gICAgICogd2UgY2xv c2UgdGhlIHNvY2tldCwgb3RoZXJ3aXNlIHdlIGxlYXZlIGl0IG9wZW4gZm9yIEtlZXBBbGl2 ZSBzdXBwb3J0Ci0gICAgICovCi0gICAgaWYgKGJhY2tlbmQtPmNsb3NlIHx8IChyLT5wcm90 b19udW0gPCBIVFRQX1ZFUlNJT04oMSwxKSkpIHsKLSAgICAgICAgYmFja2VuZC0+Y2xvc2Vf b25fcmVjeWNsZSA9IDE7Ci0gICAgICAgIGFwX3NldF9tb2R1bGVfY29uZmlnKHItPmNvbm5l
Re: [Patch] Keep Alive not workwing with mod_proxy (PR38602)
On Feb 14, 2006, at 3:54 PM, Ruediger Pluem wrote: On 02/14/2006 01:51 PM, Jim Jagielski wrote: since it really does help track some things down... In the meantime, if you can send the latest patch, I'll test it here. Please find attached Changes to the previous one: 1. Diff against r377821. 2. I removed the !backend check also. I am curious about the test results. No regressions... On Head: Failed TestStat Wstat Total Fail Failed List of Failed --- t/http11/basicauth.t 31 33.33% 2 t/security/CVE-2004-0811.t84 50.00% 1-4 12 tests and 14 subtests skipped. Failed 2/64 test scripts, 96.88% okay. 5/2478 subtests failed, 99.80% okay. With the patch: Failed TestStat Wstat Total Fail Failed List of Failed --- t/http11/basicauth.t 31 33.33% 2 t/security/CVE-2004-0811.t84 50.00% 1-4 12 tests and 14 subtests skipped. Failed 2/64 test scripts, 96.88% okay. 5/2478 subtests failed, 99.80% okay.
Re: [Patch] Keep Alive not workwing with mod_proxy (PR38602)
On 02/14/2006 10:48 PM, Jim Jagielski wrote: On Feb 14, 2006, at 3:54 PM, Ruediger Pluem wrote: I am curious about the test results. No regressions... Good. Thanks for testing that fast, even on Valentine's Day :). Regards Rüdiger
Re: [Patch] Keep Alive not workwing with mod_proxy (PR38602)
This looks like a big change, and my only concern is
that the behavior changes, although it appears that
we don't know why the current behavior is the
way it is...
Anyway:
On Feb 12, 2006, at 3:53 PM, Ruediger Pluem wrote:
The real problem is that we actually *close* our connection to the
backend
after each request (see line 1512 of mod_proxy_http.c) and if it
would have
survived we would *close* it on the reusal of this connection:
Again, this appears to be specifically done that way
for a reason, but I have no idea what it would
have been :)
@@ -1504,12 +1513,6 @@
/* found the last brigade? */
if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(bb))) {
-/* if this is the last brigade, cleanup the
- * backend connection first to prevent the
- * backend server from hanging around waiting
- * for a slow client to eat these bytes
- */
-backend-close = 1;
/* signal that we must leave */
finish = TRUE;
}
This, I think provides a clue: I'm guessing we are trying
to optimize the client-Apache link, at the expense of
opening/closing sockets to the backend, or wasting
those sockets. If we had a nice connection pool, then
it would be different...
@@ -1667,26 +1659,15 @@
proxy: HTTP: serving URL %s, url);
-/* only use stored info for top-level pages. Sub requests
don't share
- * in keepalives
- */
-if (!r-main) {
-backend = (proxy_conn_rec *) ap_get_module_config(c-
conn_config,
-
proxy_http_module);
-}
/* create space for state information */
if (!backend) {
if ((status = ap_proxy_acquire_connection(proxy_function,
backend,
worker, r-
server)) != OK)
goto cleanup;
-if (!r-main) {
-ap_set_module_config(c-conn_config,
proxy_http_module, backend);
-}
}
Not sure why we would bother still having that !backend
check, since we know it's NULL. We set it to NULL :)
And this also seems to allude to the fact that the
present framework is to support pooled connections.
Not sure how the above would conflict with subrequests
Does the patched version pass the test framework?
Re: [Patch] Keep Alive not workwing with mod_proxy (PR38602)
On Feb 13, 2006, at 11:22 AM, Jim Jagielski wrote: This, I think provides a clue: I'm guessing we are trying to optimize the client-Apache link, at the expense of opening/closing sockets to the backend, or wasting those sockets. If we had a nice connection pool, then it would be different... Giving this some very quick thought, I wonder if this is associated with that bug you and I squashed awhile ago: there is no guarantee that the next kept-alive connection will go to the same backend; as such, keeping it open is wasteful and re-using it is downright wrong. We need to look to make sure that there are sufficient checks that this doesn't happen, and that might be the reason the current behavior exists...
Re: [Patch] Keep Alive not workwing with mod_proxy (PR38602)
Jim Jagielski wrote: there is no guarantee that the next kept-alive connection will go to the same backend; as such, keeping it open is wasteful and re-using it is downright wrong. Why? Why would we care which backend a request goes to, in general. And, do we not want to use keepalives as much as possible to the backends? -- Brian Akins Lead Systems Engineer CNN Internet Technologies
Re: [Patch] Keep Alive not workwing with mod_proxy (PR38602)
On Feb 13, 2006, at 12:57 PM, Brian Akins wrote: Jim Jagielski wrote: there is no guarantee that the next kept-alive connection will go to the same backend; as such, keeping it open is wasteful and re-using it is downright wrong. Why? Why would we care which backend a request goes to, in general. And, do we not want to use keepalives as much as possible to the backends? Let's assume that you have Apache setup as a proxy and furthermore it's configured so that /html goes to foo1 and /images goes to /foo2. A request comes in for /html/index.htm, and gets proxied to foo1, as it should; the connection is kept-alive, and a request for /images/blarf.gif is requested; this should not be sent to the just kept-alive server, but instead to foo2... So we need to ensure that this doesn't happen. We had a similar type bug when picking workers out, which was patched in 349723...
Re: [Patch] Keep Alive not workwing with mod_proxy (PR38602)
Ruediger Pluem wrote: Currently I work on PR 38602 (http://issues.apache.org/bugzilla/show_bug.cgi?id=38602). First of all the reporter is correct that we do not sent the Connection: Keep-Alive header on our HTTP/1.1 keep-alive connections to the backend. But this is only the small part of the problem since 8.1.2 of the RFC says: A significant difference between HTTP/1.1 and earlier versions of HTTP is that persistent connections are the default behavior of any HTTP connection. That is, unless otherwise indicated, the client SHOULD assume that the server will maintain a persistent connection, The real problem is that we've never paid attention to the backend server. If speaking to a backend http/1.0 server, we can try connection: keep-alive if the server pays attention to it. That header is invalid for http/1.1 backends, and we should choose connection: close where appropriate. To a backend http/1.0 server, connection: close is meaningless (and wrong).
Re: [Patch] Keep Alive not workwing with mod_proxy (PR38602)
On Feb 13, 2006, at 1:28 PM, William A. Rowe, Jr. wrote: Ruediger Pluem wrote: Currently I work on PR 38602 (http://issues.apache.org/bugzilla/ show_bug.cgi?id=38602). First of all the reporter is correct that we do not sent the Connection: Keep-Alive header on our HTTP/1.1 keep-alive connections to the backend. But this is only the small part of the problem since 8.1.2 of the RFC says: A significant difference between HTTP/1.1 and earlier versions of HTTP is that persistent connections are the default behavior of any HTTP connection. That is, unless otherwise indicated, the client SHOULD assume that the server will maintain a persistent connection, The real problem is that we've never paid attention to the backend server. If speaking to a backend http/1.0 server, we can try connection: keep-alive if the server pays attention to it. That header is invalid for http/1.1 backends, and we should choose connection: close where appropriate. To a backend http/1.0 server, connection: close is meaningless (and wrong). Some sort of proxy-connection pool would help here, as that would be part of the meta-data. I was always wondering how much of DBD could be re-used for this, but wanted to wait until DBD was at a stable stage...
Re: [Patch] Keep Alive not workwing with mod_proxy (PR38602)
Jim Jagielski wrote: Let's assume that you have Apache setup as a proxy and furthermore it's configured so that /html goes to foo1 and /images goes to /foo2. A request comes in for /html/index.htm, and gets proxied to foo1, as it should; the connection is kept-alive, and a request for /images/blarf.gif is requested; this should not be sent to the just kept-alive server, but instead to foo2... I see now. Does this apply even when using balancer? I mean, do we break the keep alive to backend? We should need to... -- Brian Akins Lead Systems Engineer CNN Internet Technologies
Re: [Patch] Keep Alive not workwing with mod_proxy (PR38602)
Brian Akins wrote: Jim Jagielski wrote: Let's assume that you have Apache setup as a proxy and furthermore it's configured so that /html goes to foo1 and /images goes to /foo2. A request comes in for /html/index.htm, and gets proxied to foo1, as it should; the connection is kept-alive, and a request for /images/blarf.gif is requested; this should not be sent to the just kept-alive server, but instead to foo2... I see now. Does this apply even when using balancer? I mean, do we break the keep alive to backend? We should need to... Yep, and that's why I think we close the connection each time; we can't be assured that the next request will be for that backend, so we have a held-open socket for who knows how long, and without real connection pooling, we're wasting sockets. -- === Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/ If you can dodge a wrench, you can dodge a ball.
Re: [Patch] Keep Alive not workwing with mod_proxy (PR38602)
Jim Jagielski wrote: Yep, and that's why I think we close the connection each time; umm, I thought the balancer would try to keep the connection open to backends? A single client may wind up talking to multiple backend pools over the course of a connection (/css - A, /images - B, etc.). we're wasting sockets. we'd be saving start up time on each socket. So just to be clear, there is no connection pooling in proxy_balancer, or is there? Did I imagine that it was supposed to be? -- Brian Akins Lead Systems Engineer CNN Internet Technologies
Re: [Patch] Keep Alive not workwing with mod_proxy (PR38602)
On 02/13/2006 07:28 PM, William A. Rowe, Jr. wrote: Ruediger Pluem wrote: The real problem is that we've never paid attention to the backend server. If speaking to a backend http/1.0 server, we can try connection: keep-alive if the server pays attention to it. That header is invalid for http/1.1 Out of interest which part of which rfc (2616 / 2068) says that connection: keep-alive is invalid for http/1.1? I see this header send frequently by browsers that speak http/1.1 (of course this does not mean that this is rfc compliant :-) backends, and we should choose connection: close where appropriate. To a backend http/1.0 server, connection: close is meaningless (and wrong). Agreed. But I actually only do this when I sent an HTTP/1.1 request. BTW: This is how the code currently works and this is not changed by my patch. Another question: Do you see the need for some kind of detection code on the first request to a backend server whether it speaks HTTP/1.1 or only HTTP/1.0? Regards Rüdiger
Re: [Patch] Keep Alive not workwing with mod_proxy (PR38602)
On 02/13/2006 09:12 PM, Jim Jagielski wrote: Brian Akins wrote: Jim Jagielski wrote: Let's assume that you have Apache setup as a proxy and furthermore it's configured so that /html goes to foo1 and /images goes to /foo2. A request comes in for /html/index.htm, and gets proxied to foo1, as it should; the connection is kept-alive, and a request for /images/blarf.gif is requested; this should not be sent to the just kept-alive server, but instead to foo2... I see now. Does this apply even when using balancer? I mean, do we break the keep alive to backend? We should need to... Yep, and that's why I think we close the connection each time; we can't be assured that the next request will be for that backend, so we have a held-open socket for who knows how long, and without real connection pooling, we're wasting sockets. What do you mean by real connection pooling? We actually have connection pooling via the apr reslist. The patch ensures that we return this connection to the pool such that it can be used by other clients that use this worker. Furthermore we have to keep the following two things in mind: 1. Closing the connection in proxy_util.c is badness as other protocols (e.g. ajp are designed to have very long living connections to the backend. If we keep on closing this, it would mean that mod_proxy_ajp would be no real alternative to mod_jk. 2. To be honest my view on the proxy code might be too reverse proxy driven, but on a busy reverse proxy the connection will be quickly reused by another client, so it makes perfect sense to me to keep it open. One critical thing I currently see on this path is a race condition with the keep-alive timeout timer on the backend. The backend could close the connection after we checked it in ap_proxy_connect_backend and before we sent the request in ap_proxy_http_request. That might be also an argument to sent a Connection: Keep-Alive header and evaluate the timeout token of a possibly returned Keep-Alive header by the backend. E.g. we could restablish the connection once we know that there is less equal then one second left before the timeout happens on the backend. Regards Rüdiger
Re: [Patch] Keep Alive not workwing with mod_proxy (PR38602)
Brian Akins wrote: Jim Jagielski wrote: Yep, and that's why I think we close the connection each time; umm, I thought the balancer would try to keep the connection open to backends? A single client may wind up talking to multiple backend pools over the course of a connection (/css - A, /images - B, etc.). we're wasting sockets. we'd be saving start up time on each socket. So just to be clear, there is no connection pooling in proxy_balancer, or is there? Did I imagine that it was supposed to be? It's *kind* of there, but not really fully fleshed out... I think of a connection pool as one shared among all entities and with longer longevity that what we currently do (tuck them away in proxy_conn_rec). -- === Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/ If you can dodge a wrench, you can dodge a ball.
Re: [Patch] Keep Alive not workwing with mod_proxy (PR38602)
Ruediger Pluem wrote: What do you mean by real connection pooling? We actually have connection pooling via the apr reslist. The patch ensures that we return this connection to the pool such that it can be used by other clients that use this worker. That's what I mean by real... it's not really being used as real connection pools would be. :) Furthermore we have to keep the following two things in mind: 1. Closing the connection in proxy_util.c is badness as other protocols (e.g. ajp are designed to have very long living connections to the backend. If we keep on closing this, it would mean that mod_proxy_ajp would be no real alternative to mod_jk. +1 -- === Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/ If you can dodge a wrench, you can dodge a ball.
Re: [Patch] Keep Alive not workwing with mod_proxy (PR38602)
On 02/13/2006 05:22 PM, Jim Jagielski wrote: This looks like a big change, and my only concern is This is why I discuss it first, before I commit it :-) that the behavior changes, although it appears that we don't know why the current behavior is the way it is... Then we should either find out or adjust it to the behaviour that we think is correct as the current behaviour doesn't seem to be. Not sure why we would bother still having that !backend check, since we know it's NULL. We set it to NULL :) Well spotted :-). I missed that. I keep this in mind and will add it to the patch once we have discussed and cleared the real hard stuff. And this also seems to allude to the fact that the present framework is to support pooled connections. Not sure how the above would conflict with subrequests Good question. Does anybody remember why the old code insisted of having a fresh connection by all means for a subrequest? Does the patched version pass the test framework? Have not checked so far. I did not manage to get the test framework running on my box so far. Can someone who has it running give it a try? That would be very nice. Regards Rüdiger
