From 94802a0673637dd1f6b5c6efa5d9e31885f069e8 Mon Sep 17 00:00:00 2001 From: Graham Leggett Date: Thu, 21 Feb 2002 06:03:08 +0000 Subject: [PATCH] When proxy enabled a slow frontend client to read from an expensive backend server, it would wait until it had delivered the response to the slow frontend client completely before closing the backend connection. The backend connection is now closed as soon as the last byte is read from it, freeing up resources that would have been tied up unnecessarily. The proxy code read chunks from the backend server in a hardcoded amount of 8k. The existing ProxyReceiveBufferSize parameter has been overloaded to specify the size of this buffer. PR: Obtained from: Submitted by: Reviewed by: git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/1.3.x@93526 13f79535-47bb-0310-9956-ffa450edef68 --- src/CHANGES | 13 +++++++++++ src/modules/proxy/mod_proxy.h | 2 +- src/modules/proxy/proxy_cache.c | 18 +++++++-------- src/modules/proxy/proxy_ftp.c | 41 ++++++++++++++++++++------------- src/modules/proxy/proxy_http.c | 6 ++--- src/modules/proxy/proxy_util.c | 34 +++++++++++++++++++++++---- 6 files changed, 80 insertions(+), 34 deletions(-) diff --git a/src/CHANGES b/src/CHANGES index c342e0f87dc..c63de7b0af8 100644 --- a/src/CHANGES +++ b/src/CHANGES @@ -1,5 +1,18 @@ Changes with Apache 1.3.24 + *) When proxy enabled a slow frontend client to read from an + expensive backend server, it would wait until it had delivered + the response to the slow frontend client completely before + closing the backend connection. The backend connection is now + closed as soon as the last byte is read from it, freeing up + resources that would have been tied up unnecessarily. + [Graham Leggett, Igor Sysoev ] + + *) The proxy code read chunks from the backend server in a + hardcoded amount of 8k. The existing ProxyReceiveBufferSize + parameter has been overloaded to specify the size of this buffer. + [Graham Leggett, Igor Sysoev ] + *) [Security] Prevent invalid client hostnames from appearing in the log file. If a double-reverse lookup was performed (e.g., for an "Allow from .my.domain" directive) but failed, then diff --git a/src/modules/proxy/mod_proxy.h b/src/modules/proxy/mod_proxy.h index 3874191eb6c..0786e9e6063 100644 --- a/src/modules/proxy/mod_proxy.h +++ b/src/modules/proxy/mod_proxy.h @@ -295,7 +295,7 @@ char *ap_proxy_canon_netloc(pool *p, char **const urlp, char **userp, char **passwordp, char **hostp, int *port); const char *ap_proxy_date_canon(pool *p, const char *x); table *ap_proxy_read_headers(request_rec *r, char *buffer, int size, BUFF *f); -long int ap_proxy_send_fb(BUFF *f, request_rec *r, cache_req *c, off_t len, int nowrite); +long int ap_proxy_send_fb(BUFF *f, request_rec *r, cache_req *c, off_t len, int nowrite, size_t recv_buffer_size); void ap_proxy_write_headers(cache_req *c, const char *respline, table *t); int ap_proxy_liststr(const char *list, const char *key, char **val); void ap_proxy_hash(const char *it, char *val, int ndepth, int nlength); diff --git a/src/modules/proxy/proxy_cache.c b/src/modules/proxy/proxy_cache.c index b3ebcc86139..96e58444253 100644 --- a/src/modules/proxy/proxy_cache.c +++ b/src/modules/proxy/proxy_cache.c @@ -788,8 +788,7 @@ int ap_proxy_cache_conditional(request_rec *r, cache_req *c, BUFF *cachefp) /* if cache file is being updated */ if (c->origfp) { ap_proxy_write_headers(c, c->resp_line, c->hdrs); - ap_proxy_send_fb(c->origfp, r, c, c->len, 1); - ap_pclosef(r->pool, ap_bfileno(c->origfp, B_WR)); + ap_proxy_send_fb(c->origfp, r, c, c->len, 1, IOBUFSIZE); ap_proxy_cache_tidy(c); } else @@ -859,8 +858,7 @@ int ap_proxy_cache_conditional(request_rec *r, cache_req *c, BUFF *cachefp) /* are we updating the cache file? */ if (c->origfp) { ap_proxy_write_headers(c, c->resp_line, c->hdrs); - ap_proxy_send_fb(c->origfp, r, c, c->len, 1); - ap_pclosef(r->pool, ap_bfileno(c->origfp, B_WR)); + ap_proxy_send_fb(c->origfp, r, c, c->len, 1, IOBUFSIZE); ap_proxy_cache_tidy(c); } else @@ -893,17 +891,19 @@ int ap_proxy_cache_conditional(request_rec *r, cache_req *c, BUFF *cachefp) /* are we rewriting the cache file? */ if (c->origfp) { ap_proxy_write_headers(c, c->resp_line, c->hdrs); - ap_proxy_send_fb(c->origfp, r, c, c->len, r->header_only); - ap_pclosef(r->pool, ap_bfileno(c->origfp, B_WR)); + ap_proxy_send_fb(c->origfp, r, c, c->len, r->header_only, IOBUFSIZE); ap_proxy_cache_tidy(c); return OK; } /* no, we not */ - if (!r->header_only) - ap_proxy_send_fb(cachefp, r, NULL, c->len, 0); + if (!r->header_only) { + ap_proxy_send_fb(cachefp, r, NULL, c->len, 0, IOBUFSIZE); + } + else { + ap_pclosef(r->pool, ap_bfileno(cachefp, B_WR)); + } - ap_pclosef(r->pool, ap_bfileno(cachefp, B_WR)); return OK; } diff --git a/src/modules/proxy/proxy_ftp.c b/src/modules/proxy/proxy_ftp.c index 6668378e857..045ef46381c 100644 --- a/src/modules/proxy/proxy_ftp.c +++ b/src/modules/proxy/proxy_ftp.c @@ -264,8 +264,8 @@ static int ftp_getrc_msg(BUFF *ctrl, char *msgbuf, int msglen) static long int send_dir(BUFF *data, request_rec *r, cache_req *c, char *cwd) { - char buf[IOBUFSIZE]; - char buf2[IOBUFSIZE]; + char *buf, *buf2; + size_t buf_size; char *filename; int searchidx = 0; char *searchptr = NULL; @@ -277,6 +277,11 @@ static long int send_dir(BUFF *data, request_rec *r, cache_req *c, char *cwd) char *dir, *path, *reldir, *site, *type = NULL; char *basedir = ""; /* By default, path is relative to the $HOME dir */ + /* create default sized buffers for the stuff below */ + buf_size = IOBUFSIZE; + buf = ap_palloc(r->pool, buf_size); + buf2 = ap_palloc(r->pool, buf_size); + /* Save "scheme://site" prefix without password */ site = ap_unparse_uri_components(p, &r->parsed_uri, UNP_OMITPASSWORD|UNP_OMITPATHINFO); /* ... and path without query args */ @@ -303,7 +308,7 @@ static long int send_dir(BUFF *data, request_rec *r, cache_req *c, char *cwd) path[n-1] = '\0'; /* print "ftp://host/" */ - n = ap_snprintf(buf, sizeof(buf), DOCTYPE_HTML_3_2 + n = ap_snprintf(buf, buf_size, DOCTYPE_HTML_3_2 "%s%s%s\n" "\n" "

Directory of " @@ -327,7 +332,7 @@ static long int send_dir(BUFF *data, request_rec *r, cache_req *c, char *cwd) else ++reldir; /* print "path/" component */ - ap_snprintf(buf, sizeof(buf), "%s/", + ap_snprintf(buf, buf_size, "%s/", basedir, ap_escape_uri(p, path), ap_escape_html(p, reldir)); @@ -340,15 +345,15 @@ static long int send_dir(BUFF *data, request_rec *r, cache_req *c, char *cwd) /* If the caller has determined the current directory, and it differs */ /* from what the client requested, then show the real name */ if (cwd == NULL || strncmp (cwd, path, strlen(cwd)) == 0) { - ap_snprintf(buf, sizeof(buf), "

\n
");
+        ap_snprintf(buf, buf_size, "\n
");
     } else {
-        ap_snprintf(buf, sizeof(buf), "\n(%s)\n
",
+        ap_snprintf(buf, buf_size, "\n(%s)\n
",
                     ap_escape_html(p, cwd));
     }
     total_bytes_sent += ap_proxy_bputs2(buf, con->client, c);
 
     while (!con->aborted) {
-        n = ap_bgets(buf, sizeof buf, data);
+        n = ap_bgets(buf, buf_size, data);
         if (n == -1) {          /* input error */
             if (c != NULL) {
                 ap_log_rerror(APLOG_MARK, APLOG_ERR, c->req,
@@ -375,12 +380,12 @@ static long int send_dir(BUFF *data, request_rec *r, cache_req *c, char *cwd)
             if (filename != buf)
                 *(filename++) = '\0';
             *(link_ptr++) = '\0';
-            ap_snprintf(buf2, sizeof(buf2), "%s %s %s\n",
+            ap_snprintf(buf2, buf_size, "%s %s %s\n",
                         ap_escape_html(p, buf),
                         ap_escape_uri(p,filename),
                         ap_escape_html(p, filename),
                         ap_escape_html(p, link_ptr));
-            ap_cpystrn(buf, buf2, sizeof(buf));
+            ap_cpystrn(buf, buf2, buf_size);
             n = strlen(buf);
         }
         /* Handle unix style or DOS style directory  */
@@ -410,23 +415,23 @@ static long int send_dir(BUFF *data, request_rec *r, cache_req *c, char *cwd)
 
             /* Special handling for '.' and '..': append slash to link */
             if (!strcmp(filename, ".") || !strcmp(filename, "..") || buf[0] == 'd') {
-                ap_snprintf(buf2, sizeof(buf2), "%s %s\n",
+                ap_snprintf(buf2, buf_size, "%s %s\n",
                             ap_escape_html(p, buf), ap_escape_uri(p,filename),
                             ap_escape_html(p, filename));
             }
             else {
-                ap_snprintf(buf2, sizeof(buf2), "%s %s\n",
+                ap_snprintf(buf2, buf_size, "%s %s\n",
                             ap_escape_html(p, buf),
                             ap_escape_uri(p,filename),
                             ap_escape_html(p, filename));
             }
-            ap_cpystrn(buf, buf2, sizeof(buf));
+            ap_cpystrn(buf, buf2, buf_size);
             n = strlen(buf);
         }
         /* else??? What about other OS's output formats? */
         else {
             strcat(buf, "\n"); /* re-append the newline char */
-            ap_cpystrn(buf, ap_escape_html(p, buf), sizeof(buf));
+            ap_cpystrn(buf, ap_escape_html(p, buf), buf_size);
         }
 
         total_bytes_sent += ap_proxy_bputs2(buf, con->client, c);
@@ -438,6 +443,8 @@ static long int send_dir(BUFF *data, request_rec *r, cache_req *c, char *cwd)
     total_bytes_sent += ap_proxy_bputs2(ap_psignature("", r), con->client, c);
     total_bytes_sent += ap_proxy_bputs2("\n", con->client, c);
 
+    ap_bclose(data);
+
     ap_bflush(con->client);
 
     return total_bytes_sent;
@@ -1341,10 +1348,12 @@ int ap_proxy_ftp_handler(request_rec *r, cache_req *c, char *url)
 /* we need to set this for ap_proxy_send_fb()... */
             if (c != NULL)
                 c->cache_completion = 0;
-            ap_proxy_send_fb(data, r, c, -1, 0);
-        } else
+            ap_proxy_send_fb(data, r, c, -1, 0, conf->recv_buffer_size);
+        }
+        else {
             send_dir(data, r, c, cwd);
-        ap_bclose(data);
+        }
+        /* ap_proxy_send_fb() closes the socket */
         data = NULL;
         dsock = -1;
 
diff --git a/src/modules/proxy/proxy_http.c b/src/modules/proxy/proxy_http.c
index 92fb6d98726..f03816110a8 100644
--- a/src/modules/proxy/proxy_http.c
+++ b/src/modules/proxy/proxy_http.c
@@ -582,12 +582,12 @@ int ap_proxy_http_handler(request_rec *r, cache_req *c, char *url,
  * content length is not known. We need to make 100% sure c->len is always
  * set correctly before we get here to correctly do keepalive.
  */
-        ap_proxy_send_fb(f, r, c, c->len, 0);
+        ap_proxy_send_fb(f, r, c, c->len, 0, conf->recv_buffer_size);
     }
 
-    ap_proxy_cache_tidy(c);
+    /* ap_proxy_send_fb() closes the socket f for us */
 
-    ap_bclose(f);
+    ap_proxy_cache_tidy(c);
 
     ap_proxy_garbage_coll(r);
     return OK;
diff --git a/src/modules/proxy/proxy_util.c b/src/modules/proxy/proxy_util.c
index 36d7ebcdb28..665417eed90 100644
--- a/src/modules/proxy/proxy_util.c
+++ b/src/modules/proxy/proxy_util.c
@@ -497,15 +497,21 @@ table *ap_proxy_read_headers(request_rec *r, char *buffer, int size, BUFF *f)
  * - r->connection->client, if nowrite == 0
  */
 
-long int ap_proxy_send_fb(BUFF *f, request_rec *r, cache_req *c, off_t len, int nowrite)
+long int ap_proxy_send_fb(BUFF *f, request_rec *r, cache_req *c, off_t len, int nowrite, size_t recv_buffer_size)
 {
     int  ok;
-    char buf[IOBUFSIZE];
+    char *buf;
+    size_t buf_size;
     long total_bytes_rcvd;
     register int n, o, w;
     conn_rec *con = r->connection;
     int alternate_timeouts = 1; /* 1 if we alternate between soft & hard timeouts */
 
+    /* allocate a buffer to store the bytes in */
+    /* make sure it is at least IOBUFSIZE, as recv_buffer_size may be zero for system default */
+    buf_size = MAX(recv_buffer_size, IOBUFSIZE);
+    buf = ap_palloc(r->pool, buf_size);
+
     total_bytes_rcvd = 0;
     if (c != NULL)
         c->written = 0;
@@ -554,10 +560,10 @@ long int ap_proxy_send_fb(BUFF *f, request_rec *r, cache_req *c, off_t len, int
 
         /* Read block from server */
         if (-1 == len) {
-            n = ap_bread(f, buf, IOBUFSIZE);
+            n = ap_bread(f, buf, buf_size);
         }
         else {
-            n = ap_bread(f, buf, MIN(IOBUFSIZE, len - total_bytes_rcvd));
+            n = ap_bread(f, buf, MIN(buf_size, len - total_bytes_rcvd));
         }
 
         if (alternate_timeouts)
@@ -578,6 +584,18 @@ long int ap_proxy_send_fb(BUFF *f, request_rec *r, cache_req *c, off_t len, int
         o = 0;
         total_bytes_rcvd += n;
 
+        /* if we've received everything... */
+        /* in the case of slow frontends and expensive backends,
+         * we want to avoid leaving a backend connection hanging
+         * while the frontend takes it's time to absorb the bytes.
+         * so: if we just read the last block, we close the backend
+         * connection now instead of later - it's no longer needed.
+         */
+        if (total_bytes_rcvd == len) {
+            ap_bclose(f);
+            f = NULL;
+        }
+
         /* Write to cache first. */
         /*@@@ XXX FIXME: Assuming that writing the cache file won't time out?!!? */
         if (c != NULL && c->fp != NULL) {
@@ -634,8 +652,14 @@ long int ap_proxy_send_fb(BUFF *f, request_rec *r, cache_req *c, off_t len, int
 
     } /* loop and ap_bread while "ok" */
 
-    if (!con->aborted)
+    /* if the backend connection is still open, close it */
+    if (f) {
+        ap_bclose(f);
+    }
+
+    if (!con->aborted) {
         ap_bflush(con->client);
+    }
 
     ap_kill_timeout(r);
     return total_bytes_rcvd;
-- 
2.47.2