You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by yl...@apache.org on 2016/06/27 17:26:13 UTC
svn commit: r1750392 - in /httpd/httpd/trunk: CHANGES
docs/log-message-tags/next-number include/ap_mmn.h
modules/proxy/mod_proxy.h modules/proxy/mod_proxy_ajp.c
modules/proxy/mod_proxy_fcgi.c modules/proxy/mod_proxy_http.c
modules/proxy/proxy_util.c
Author: ylavic
Date: Mon Jun 27 17:26:12 2016
New Revision: 1750392
URL: http://svn.apache.org/viewvc?rev=1750392&view=rev
Log:
mod_proxy_{http,ajp,fcgi}}: don't reuse backend connections with data available
before the request is sent. PR 57832.
ap_proxy_check_backend() can be used before ap_proxy_connect_backend() to try
to read available data (including from the filters), and is called by
ap_proxy_connect_backend() to check the socket state only (as before, still
relevant after ap_proxy_check_backend() due to filter data which may not have
triggered a real socket operation).
Modified:
httpd/httpd/trunk/CHANGES
httpd/httpd/trunk/docs/log-message-tags/next-number
httpd/httpd/trunk/include/ap_mmn.h
httpd/httpd/trunk/modules/proxy/mod_proxy.h
httpd/httpd/trunk/modules/proxy/mod_proxy_ajp.c
httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
httpd/httpd/trunk/modules/proxy/proxy_util.c
Modified: httpd/httpd/trunk/CHANGES
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1750392&r1=1750391&r2=1750392&view=diff
==============================================================================
--- httpd/httpd/trunk/CHANGES [utf-8] (original)
+++ httpd/httpd/trunk/CHANGES [utf-8] Mon Jun 27 17:26:12 2016
@@ -1,6 +1,9 @@
-*- coding: utf-8 -*-
Changes with Apache 2.5.0
+ *) mod_proxy_{http,ajp,fcgi}: don't reuse backend connections with data
+ available before the request is sent. PR 57832. [Yann Ylavic]
+
*) mod_sed: Fix 'x' command processing. [Christophe Jaillet]
*) core: Drop an invalid Last-Modified header value coming
Modified: httpd/httpd/trunk/docs/log-message-tags/next-number
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/docs/log-message-tags/next-number?rev=1750392&r1=1750391&r2=1750392&view=diff
==============================================================================
--- httpd/httpd/trunk/docs/log-message-tags/next-number (original)
+++ httpd/httpd/trunk/docs/log-message-tags/next-number Mon Jun 27 17:26:12 2016
@@ -1 +1 @@
-3408
+3409
Modified: httpd/httpd/trunk/include/ap_mmn.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/include/ap_mmn.h?rev=1750392&r1=1750391&r2=1750392&view=diff
==============================================================================
--- httpd/httpd/trunk/include/ap_mmn.h (original)
+++ httpd/httpd/trunk/include/ap_mmn.h Mon Jun 27 17:26:12 2016
@@ -532,6 +532,8 @@
* dav_success_proppatch.
* 20160608.4 (2.5.0-dev) Add dav_acl_provider, dav_acl_provider_register
* dav_get_acl_providers.
+ * 20160608.5 (2.5.0-dev) Add ap_proxy_check_backend(), and tmp_bb to struct
+ * proxy_conn_rec.
*/
#define MODULE_MAGIC_COOKIE 0x41503235UL /* "AP25" */
@@ -539,7 +541,7 @@
#ifndef MODULE_MAGIC_NUMBER_MAJOR
#define MODULE_MAGIC_NUMBER_MAJOR 20160608
#endif
-#define MODULE_MAGIC_NUMBER_MINOR 4 /* 0...n */
+#define MODULE_MAGIC_NUMBER_MINOR 5 /* 0...n */
/**
* Determine if the server's current MODULE_MAGIC_NUMBER is at least a
Modified: httpd/httpd/trunk/modules/proxy/mod_proxy.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy.h?rev=1750392&r1=1750391&r2=1750392&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/proxy/mod_proxy.h (original)
+++ httpd/httpd/trunk/modules/proxy/mod_proxy.h Mon Jun 27 17:26:12 2016
@@ -271,6 +271,7 @@ typedef struct {
unsigned int inreslist:1; /* connection in apr_reslist? */
const char *uds_path; /* Unix domain socket path */
const char *ssl_hostname;/* Hostname (SNI) in use by SSL connection */
+ apr_bucket_brigade *tmp_bb;
} proxy_conn_rec;
typedef struct {
@@ -973,6 +974,20 @@ PROXY_DECLARE(int) ap_proxy_release_conn
proxy_conn_rec *conn,
server_rec *s);
/**
+ * Check a connection to the backend
+ * @param proxy_function calling proxy scheme (http, ajp, ...)
+ * @param conn acquired connection
+ * @param s current server record
+ * @param expect_empty whether to check for empty (no data available) or not
+ * @return APR_SUCCESS or error status (APR_ENOTEMPTY if expect_empty
+ * is set but the connection is not empty)
+ */
+PROXY_DECLARE(apr_status_t) ap_proxy_check_backend(const char *proxy_function,
+ proxy_conn_rec *conn,
+ server_rec *s,
+ int expect_empty);
+
+/**
* Make a connection to the backend
* @param proxy_function calling proxy scheme (http, ajp, ...)
* @param conn acquired connection
Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_ajp.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_ajp.c?rev=1750392&r1=1750391&r2=1750392&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/proxy/mod_proxy_ajp.c (original)
+++ httpd/httpd/trunk/modules/proxy/mod_proxy_ajp.c Mon Jun 27 17:26:12 2016
@@ -783,6 +783,7 @@ static int proxy_ajp_handler(request_rec
break;
/* Step Two: Make the Connection */
+ ap_proxy_check_backend(scheme, backend, r->server, 1);
if (ap_proxy_connect_backend(scheme, backend, worker, r->server)) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(00896)
"failed to make connection to backend: %s",
Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c?rev=1750392&r1=1750391&r2=1750392&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c (original)
+++ httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c Mon Jun 27 17:26:12 2016
@@ -935,6 +935,7 @@ static int proxy_fcgi_handler(request_re
*/
backend->close = 1;
if (worker->s->disablereuse_set && !worker->s->disablereuse) {
+ ap_proxy_check_backend(FCGI_SCHEME, backend, r->server, 1);
backend->close = 0;
}
Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_http.c?rev=1750392&r1=1750391&r2=1750392&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/proxy/mod_proxy_http.c (original)
+++ httpd/httpd/trunk/modules/proxy/mod_proxy_http.c Mon Jun 27 17:26:12 2016
@@ -2073,6 +2073,7 @@ static int proxy_http_handler(request_re
}
/* Step Two: Make the Connection */
+ ap_proxy_check_backend(proxy_function, backend, r->server, 1);
if (ap_proxy_connect_backend(proxy_function, backend, worker, r->server)) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01114)
"HTTP: failed to make connection to backend: %s",
Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?rev=1750392&r1=1750391&r2=1750392&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
+++ httpd/httpd/trunk/modules/proxy/proxy_util.c Mon Jun 27 17:26:12 2016
@@ -2699,13 +2699,81 @@ PROXY_DECLARE(apr_status_t) ap_proxy_con
#endif
}
+PROXY_DECLARE(apr_status_t) ap_proxy_check_backend(const char *proxy_function,
+ proxy_conn_rec *conn,
+ server_rec *s,
+ int expect_empty)
+{
+ apr_status_t rv;
+
+ if (!conn->sock) {
+ return APR_ENOTSOCK;
+ }
+
+ if (conn->connection) {
+ conn_rec *c = conn->connection;
+ if (conn->tmp_bb == NULL) {
+ conn->tmp_bb = apr_brigade_create(c->pool, c->bucket_alloc);
+ }
+ rv = ap_get_brigade(c->input_filters, conn->tmp_bb,
+ AP_MODE_SPECULATIVE, APR_NONBLOCK_READ, 1);
+ if (rv == APR_SUCCESS && expect_empty) {
+ apr_off_t len = 0;
+ apr_brigade_length(conn->tmp_bb, 0, &len);
+ if (len) {
+ rv = APR_ENOTEMPTY;
+ }
+ }
+ else if (APR_STATUS_IS_EAGAIN(rv)) {
+ rv = APR_SUCCESS;
+ }
+ apr_brigade_cleanup(conn->tmp_bb);
+ }
+ else if (ap_proxy_is_socket_connected(conn->sock)) {
+ rv = APR_SUCCESS;
+ }
+ else {
+ rv = APR_EPIPE;
+ }
+
+ if (rv != APR_SUCCESS) {
+ /* This clears conn->scpool (and associated data), so backup and
+ * restore any ssl_hostname for this connection set earlier by
+ * ap_proxy_determine_connection().
+ */
+ char ssl_hostname[PROXY_WORKER_RFC1035_NAME_SIZE];
+ if (!conn->ssl_hostname || PROXY_STRNCPY(ssl_hostname,
+ conn->ssl_hostname)) {
+ ssl_hostname[0] = '\0';
+ }
+
+ socket_cleanup(conn);
+ if (rv != APR_ENOTEMPTY) {
+ ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(00951)
+ "%s: backend socket is disconnected.",
+ proxy_function);
+ }
+ else {
+ ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(03408)
+ "%s: reusable backend connection is not empty: "
+ "forcibly closed", proxy_function);
+ }
+
+ if (ssl_hostname[0]) {
+ conn->ssl_hostname = apr_pstrdup(conn->scpool, ssl_hostname);
+ }
+ }
+
+ return rv;
+}
+
PROXY_DECLARE(int) ap_proxy_connect_backend(const char *proxy_function,
proxy_conn_rec *conn,
proxy_worker *worker,
server_rec *s)
{
apr_status_t rv;
- int connected = 0;
+ int connected;
int loglevel;
apr_sockaddr_t *backend_addr = conn->addr;
/* the local address to use for the outgoing connection */
@@ -2715,28 +2783,8 @@ PROXY_DECLARE(int) ap_proxy_connect_back
proxy_server_conf *conf =
(proxy_server_conf *) ap_get_module_config(sconf, &proxy_module);
- if (conn->sock) {
- if (!(connected = ap_proxy_is_socket_connected(conn->sock))) {
- /* This clears conn->scpool (and associated data), so backup and
- * restore any ssl_hostname for this connection set earlier by
- * ap_proxy_determine_connection().
- */
- char ssl_hostname[PROXY_WORKER_RFC1035_NAME_SIZE];
- if (!conn->ssl_hostname || PROXY_STRNCPY(ssl_hostname,
- conn->ssl_hostname)) {
- ssl_hostname[0] = '\0';
- }
-
- socket_cleanup(conn);
- ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(00951)
- "%s: backend socket is disconnected.",
- proxy_function);
-
- if (ssl_hostname[0]) {
- conn->ssl_hostname = apr_pstrdup(conn->scpool, ssl_hostname);
- }
- }
- }
+ connected = (ap_proxy_check_backend(proxy_function, conn,
+ s, 0) == APR_SUCCESS);
while ((backend_addr || conn->uds_path) && !connected) {
#if APR_HAVE_SYS_UN_H
if (conn->uds_path)
Re: svn commit: r1750392 - in /httpd/httpd/trunk: CHANGES
docs/log-message-tags/next-number include/ap_mmn.h modules/proxy/mod_proxy.h
modules/proxy/mod_proxy_ajp.c modules/proxy/mod_proxy_fcgi.c
modules/proxy/mod_proxy_http.c modules/proxy/proxy_util.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Jun 28, 2016 at 1:52 PM, Jim Jagielski <ji...@jagunet.com> wrote:
>
>> On Jun 27, 2016, at 4:25 PM, Yann Ylavic <yl...@gmail.com> wrote:
>>
>> On Mon, Jun 27, 2016 at 9:05 PM, Jim Jagielski <ji...@jagunet.com> wrote:
>>>
>>>> On Jun 27, 2016, at 1:26 PM, ylavic@apache.org wrote:
>>>>
>>>> + apr_bucket_brigade *tmp_bb;
>>>> } proxy_conn_rec;
>>>>
>>>
>>> I am missing the reason why this brigade needs to be
>>> a field in this struct. Is it simply to prevent us having
>>> to create it during each call of ap_proxy_check_backend()?
>>
>> Yes, mainly, we could create/destroy (yet another) temporary brigade
>> in ap_proxy_check_backend(), but there are several places already
>> where we do that (and where the proxy_conn_rec is available).
>> I thought we could later follow up on this change and optimize these
>> by using the new tmp_bb field (cleanup is faster than
>> create/destroy)...
>
> Oh. Gotcha. +1. We should make sure to document it as such.
> If there is data in the brigade between function calls, we
> will get some weird behavior :)
Good point, will do.
Re: svn commit: r1750392 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number include/ap_mmn.h modules/proxy/mod_proxy.h modules/proxy/mod_proxy_ajp.c modules/proxy/mod_proxy_fcgi.c modules/proxy/mod_proxy_http.c modules/proxy/proxy_util.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
> On Jun 27, 2016, at 4:25 PM, Yann Ylavic <yl...@gmail.com> wrote:
>
> On Mon, Jun 27, 2016 at 9:05 PM, Jim Jagielski <ji...@jagunet.com> wrote:
>>
>>> On Jun 27, 2016, at 1:26 PM, ylavic@apache.org wrote:
>>>
>>> + apr_bucket_brigade *tmp_bb;
>>> } proxy_conn_rec;
>>>
>>
>> I am missing the reason why this brigade needs to be
>> a field in this struct. Is it simply to prevent us having
>> to create it during each call of ap_proxy_check_backend()?
>
> Yes, mainly, we could create/destroy (yet another) temporary brigade
> in ap_proxy_check_backend(), but there are several places already
> where we do that (and where the proxy_conn_rec is available).
> I thought we could later follow up on this change and optimize these
> by using the new tmp_bb field (cleanup is faster than
> create/destroy)...
Oh. Gotcha. +1. We should make sure to document it as such.
If there is data in the brigade between function calls, we
will get some weird behavior :)
Re: svn commit: r1750392 - in /httpd/httpd/trunk: CHANGES
docs/log-message-tags/next-number include/ap_mmn.h modules/proxy/mod_proxy.h
modules/proxy/mod_proxy_ajp.c modules/proxy/mod_proxy_fcgi.c
modules/proxy/mod_proxy_http.c modules/proxy/proxy_util.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Jun 27, 2016 at 10:25 PM, Yann Ylavic <yl...@gmail.com> wrote:
> I thought we could later follow up on this change and optimize these
> by using the new tmp_bb field (cleanup is faster than
> create/destroy)...
Something like the attached patch.
Re: svn commit: r1750392 - in /httpd/httpd/trunk: CHANGES
docs/log-message-tags/next-number include/ap_mmn.h modules/proxy/mod_proxy.h
modules/proxy/mod_proxy_ajp.c modules/proxy/mod_proxy_fcgi.c
modules/proxy/mod_proxy_http.c modules/proxy/proxy_util.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Jun 27, 2016 at 9:05 PM, Jim Jagielski <ji...@jagunet.com> wrote:
>
>> On Jun 27, 2016, at 1:26 PM, ylavic@apache.org wrote:
>>
>> + apr_bucket_brigade *tmp_bb;
>> } proxy_conn_rec;
>>
>
> I am missing the reason why this brigade needs to be
> a field in this struct. Is it simply to prevent us having
> to create it during each call of ap_proxy_check_backend()?
Yes, mainly, we could create/destroy (yet another) temporary brigade
in ap_proxy_check_backend(), but there are several places already
where we do that (and where the proxy_conn_rec is available).
I thought we could later follow up on this change and optimize these
by using the new tmp_bb field (cleanup is faster than
create/destroy)...
Re: svn commit: r1750392 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number include/ap_mmn.h modules/proxy/mod_proxy.h modules/proxy/mod_proxy_ajp.c modules/proxy/mod_proxy_fcgi.c modules/proxy/mod_proxy_http.c modules/proxy/proxy_util.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
> On Jun 27, 2016, at 1:26 PM, ylavic@apache.org wrote:
>
> + apr_bucket_brigade *tmp_bb;
> } proxy_conn_rec;
>
I am missing the reason why this brigade needs to be
a field in this struct. Is it simply to prevent us having
to create it during each call of ap_proxy_check_backend()?