Sorry for the delayed reply. At the moment I don't have time to look at the
patch proposal in detail, sorry about that too. I'll get back to it soon, I
hope.
> Pre-fetching 16K (or waiting for input filters to provide these) is not
always a fast operation, and the case where the backend closes its
connection in the meantime is easy to
> reproduce (and even likely to happen with some applications).
I absolutely agree. That's why I proposed to solve this more systematically
then trial-and-error-like in the first place.
> Hence, why cannot mod_proxy_http prefetch the client's body *before*
trying anything with the backend connection, and only if it's all good,
connect (or reuse a connection to) the
> backend and then start forwarding the data immediatly (flushing the
prefetched ones) ?
Now I'm confused. Are we talking about flushing everything immediately from
client to backend (that's what I understood from your first patch proposal
in this thread) or are we talking about pre fetching the whole request body
from the client and then passing it on to the backend as a whole ?
The problem I mentioned in the first message is about treating the backend
connection in an error prone way. By not ensuring the connection is still
valid - e.g. as seen by the peer - we risk running into this problem no
matter what we do elsewhere. So I really think the proper way to handle
this is to reopen the connection if necessary - be this with a filter or
integrated into the connection handling itself - and only fail after we
tried at least once.
Re-thinking the proposed flush mechanism, I don't think using the flushing
will really solve the problem, albeit it probably narrowing down the window
of opportunity for the problem significantly in most scenarios. I mention
this because I'm getting the feeling two different discussions are being
mixed up: Solving the "keep-alive time out" problem and introducing the
"flush-all-immediately" option. While the latter might improve the
situation with the first, it is no complete solution and there are a lot of
scenarios where it does not apply due to other factors like filters (as
discussed previously).
The flush option itself sounds like a good idea, so I see no reason why not
to put it in. I just don't want to loose focus on the actual problem ;-)
Cheers,
Thomas
On Mon, Oct 7, 2013 at 6:49 PM, Yann Ylavic <ylavic....@gmail.com> wrote:
> Sorry to insist about this issue but I don't see how the current
> mod_proxy_http's behaviour of connecting the backend (or checking
> established connection reusability) before prefetching the client's body is
> not a problem.
>
> Pre-fetching 16K (or waiting for input filters to provide these) is not
> always a fast operation, and the case where the backend closes its
> connection in the meantime is easy to reproduce (and even likely to happen
> with some applications).
>
> Hence, why cannot mod_proxy_http prefetch the client's body *before*
> trying anything with the backend connection, and only if it's all good,
> connect (or reuse a connection to) the backend and then start forwarding
> the data immediatly (flushing the prefetched ones) ?
>
> By doing this, the time between the connect (or check) and the first bytes
> sent to the backend is minimal.
>
> It does not require to disable the prefetch since this one can really help
> the decision about forwarding Content-Length vs chunked (which may not be
> handled by the backend, IFAIK this is not a HTTP/1.1 requirement to handle
> it for requests).
>
> That was the purpose of the first patch I proposed ealier, but had no
> feedback...
> Maybe this message and the following patch could have more chance.
>
> This patch is quite the same as the previous one (ap_proxy_http_request
> split in 2, ap_proxy_http_prefetch to prefetch before connect and
> ap_proxy_http_request to forward using the relevent
> stream_reqbody_cl/chunked once prefetched and connected). It just fixes the
> immediate flush of the prefetched bytes when it's time to forward, and the
> handling of backend->close set by ap_proxy_create_hdrbrgd (or
> ap_proxy_http_prefetch) before the connection even exist (or the previous
> one to be reused).
>
> Regards,
> Yann.
>
> Index: modules/proxy/mod_proxy_http.c
> ===================================================================
> --- modules/proxy/mod_proxy_http.c (revision 1529127)
> +++ modules/proxy/mod_proxy_http.c (working copy)
> @@ -251,6 +251,7 @@ static int stream_reqbody_chunked(apr_pool_t *p,
> while (!APR_BUCKET_IS_EOS(APR_BRIGADE_FIRST(input_brigade)))
> {
> char chunk_hdr[20]; /* must be here due to transient bucket. */
> + int flush = 0;
>
> /* If this brigade contains EOS, either stop or remove it. */
> if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(input_brigade))) {
> @@ -299,13 +300,14 @@ static int stream_reqbody_chunked(apr_pool_t *p,
> }
>
> header_brigade = NULL;
> + flush = 1;
> }
> else {
> bb = input_brigade;
>
> }
>
> /* The request is flushed below this loop with chunk EOS header */
> - rv = ap_proxy_pass_brigade(bucket_alloc, r, p_conn, origin, bb,
> 0);
> + rv = ap_proxy_pass_brigade(bucket_alloc, r, p_conn, origin, bb,
> flush);
>
> if (rv != OK) {
> return rv;
> }
> @@ -389,12 +391,14 @@ static int stream_reqbody_cl(apr_pool_t *p,
>
> while (!APR_BUCKET_IS_EOS(APR_BRIGADE_FIRST(input_brigade)))
> {
> + int flush = 0;
> +
> apr_brigade_length(input_brigade, 1, &bytes);
> bytes_streamed += bytes;
>
> /* If this brigade contains EOS, either stop or remove it. */
> if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(input_brigade))) {
> - seen_eos = 1;
> + seen_eos = flush = 1;
>
> /* We can't pass this EOS to the output_filters. */
> e = APR_BRIGADE_LAST(input_brigade);
> @@ -444,13 +448,14 @@ static int stream_reqbody_cl(apr_pool_t *p,
> }
>
> header_brigade = NULL;
> + flush = 1;
> }
> else {
> bb = input_brigade;
>
> }
>
> /* Once we hit EOS, we are ready to flush. */
> - rv = ap_proxy_pass_brigade(bucket_alloc, r, p_conn, origin, bb,
> seen_eos);
> + rv = ap_proxy_pass_brigade(bucket_alloc, r, p_conn, origin, bb,
> flush);
>
> if (rv != OK) {
> return rv ;
> }
> @@ -677,29 +682,33 @@ static apr_status_t proxy_buckets_lifetime_transfo
>
> return rv;
> }
>
> +enum rb_methods {
> + RB_INIT,
> + RB_STREAM_CL,
> + RB_STREAM_CHUNKED,
> + RB_SPOOL_CL
> +};
> +
> static
> -int ap_proxy_http_request(apr_pool_t *p, request_rec *r,
> +int ap_proxy_http_prefetch(apr_pool_t *p, request_rec *r,
> proxy_conn_rec *p_conn, proxy_worker
> *worker,
> proxy_server_conf *conf,
> apr_uri_t *uri,
> - char *url, char *server_portstr)
> + char *url, char *server_portstr,
> + apr_bucket_brigade *header_brigade,
> + apr_bucket_brigade *input_brigade,
> + char **old_cl_val, char **old_te_val,
> + enum rb_methods *rb_method)
>
> {
> conn_rec *c = r->connection;
> apr_bucket_alloc_t *bucket_alloc = c->bucket_alloc;
> - apr_bucket_brigade *header_brigade;
> - apr_bucket_brigade *input_brigade;
> apr_bucket_brigade *temp_brigade;
> apr_bucket *e;
> char *buf;
> apr_status_t status;
> - enum rb_methods {RB_INIT, RB_STREAM_CL, RB_STREAM_CHUNKED,
> RB_SPOOL_CL};
> - enum rb_methods rb_method = RB_INIT;
> - char *old_cl_val = NULL;
> - char *old_te_val = NULL;
> apr_off_t bytes_read = 0;
> apr_off_t bytes;
> int force10, rv;
> - conn_rec *origin = p_conn->connection;
>
> if (apr_table_get(r->subprocess_env, "force-proxy-request-1.0")) {
> if (r->expecting_100) {
> @@ -710,17 +719,13 @@ static
>
> force10 = 0;
> }
>
> - header_brigade = apr_brigade_create(p, origin->bucket_alloc);
> rv = ap_proxy_create_hdrbrgd(p, header_brigade, r, p_conn,
> worker, conf, uri, url, server_portstr,
> - &old_cl_val, &old_te_val);
> + old_cl_val, old_te_val);
> if (rv != OK) {
> return rv;
> }
>
> - /* We have headers, let's figure out our request body... */
> - input_brigade = apr_brigade_create(p, bucket_alloc);
> -
> /* sub-requests never use keepalives, and mustn't pass request bodies.
> * Because the new logic looks at input_brigade, we will
> self-terminate
> * input_brigade and jump past all of the request body logic...
> @@ -733,15 +738,15 @@ static
>
> if (!r->kept_body && r->main) {
> /* XXX: Why DON'T sub-requests use keepalives? */
> p_conn->close = 1;
> - if (old_cl_val) {
> - old_cl_val = NULL;
> + if (*old_cl_val) {
> + *old_cl_val = NULL;
> apr_table_unset(r->headers_in, "Content-Length");
> }
> - if (old_te_val) {
> - old_te_val = NULL;
> + if (*old_te_val) {
> + *old_te_val = NULL;
> apr_table_unset(r->headers_in, "Transfer-Encoding");
> }
> - rb_method = RB_STREAM_CL;
> + *rb_method = RB_STREAM_CL;
> e = apr_bucket_eos_create(input_brigade->bucket_alloc);
> APR_BRIGADE_INSERT_TAIL(input_brigade, e);
> goto skip_body;
> @@ -755,20 +760,19 @@ static
>
> * encoding has been done by the extensions' handler, and
> * do not modify add_te_chunked's logic
> */
> - if (old_te_val && strcasecmp(old_te_val, "chunked") != 0) {
> + if (*old_te_val && strcasecmp(*old_te_val, "chunked") != 0) {
> ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01093)
> "%s Transfer-Encoding is not supported",
> old_te_val);
> return HTTP_INTERNAL_SERVER_ERROR;
> }
>
> - if (old_cl_val && old_te_val) {
> + if (*old_cl_val && *old_te_val) {
> ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01094)
> "client %s (%s) requested Transfer-Encoding "
> "chunked body with Content-Length (C-L ignored)",
> c->client_ip, c->remote_host ? c->remote_host: "");
> apr_table_unset(r->headers_in, "Content-Length");
> - old_cl_val = NULL;
> - origin->keepalive = AP_CONN_CLOSE;
> + *old_cl_val = NULL;
> p_conn->close = 1;
> }
>
> @@ -867,42 +871,42 @@ static
>
> * If we expected no body, and read no body, do not set
> * the Content-Length.
> */
> - if (old_cl_val || old_te_val || bytes_read) {
> - old_cl_val = apr_off_t_toa(r->pool, bytes_read);
> + if (*old_cl_val || *old_te_val || bytes_read) {
> + *old_cl_val = apr_off_t_toa(r->pool, bytes_read);
> }
> - rb_method = RB_STREAM_CL;
> + *rb_method = RB_STREAM_CL;
> }
> - else if (old_te_val) {
> + else if (*old_te_val) {
> if (force10
> || (apr_table_get(r->subprocess_env, "proxy-sendcl")
> && !apr_table_get(r->subprocess_env, "proxy-sendchunks")
> && !apr_table_get(r->subprocess_env,
> "proxy-sendchunked"))) {
> - rb_method = RB_SPOOL_CL;
> + *rb_method = RB_SPOOL_CL;
> }
> else {
> - rb_method = RB_STREAM_CHUNKED;
> + *rb_method = RB_STREAM_CHUNKED;
> }
> }
> - else if (old_cl_val) {
> + else if (*old_cl_val) {
> if (r->input_filters == r->proto_input_filters) {
> - rb_method = RB_STREAM_CL;
> + *rb_method = RB_STREAM_CL;
> }
> else if (!force10
> && (apr_table_get(r->subprocess_env, "proxy-sendchunks")
> || apr_table_get(r->subprocess_env,
> "proxy-sendchunked"))
> && !apr_table_get(r->subprocess_env, "proxy-sendcl")) {
> - rb_method = RB_STREAM_CHUNKED;
> + *rb_method = RB_STREAM_CHUNKED;
> }
> else {
> - rb_method = RB_SPOOL_CL;
> + *rb_method = RB_SPOOL_CL;
> }
> }
> else {
> /* This is an appropriate default; very efficient for no-body
> * requests, and has the behavior that it will not add any C-L
> - * when the old_cl_val is NULL.
> + * when the *old_cl_val is NULL.
> */
> - rb_method = RB_SPOOL_CL;
> + *rb_method = RB_SPOOL_CL;
> }
>
> /* Yes I hate gotos. This is the subrequest shortcut */
> @@ -924,6 +928,21 @@ skip_body:
> APR_BRIGADE_INSERT_TAIL(header_brigade, e);
> }
>
>
> + return OK;
> +}
> +
> +static
> +int ap_proxy_http_request(apr_pool_t *p, request_rec *r,
> + proxy_conn_rec *p_conn,
> + apr_bucket_brigade *header_brigade,
> + apr_bucket_brigade *input_brigade,
> + char *old_cl_val, char *old_te_val,
> + enum rb_methods rb_method)
> +{
> + int rv;
> + apr_off_t bytes_read = 0;
> + conn_rec *origin = p_conn->connection;
> +
> /* send the request body, if any. */
> switch(rb_method) {
> case RB_STREAM_CHUNKED:
> @@ -935,6 +954,7 @@ skip_body:
>
> input_brigade, old_cl_val);
> break;
> case RB_SPOOL_CL:
> + apr_brigade_length(input_brigade, 1, &bytes_read);
> rv = spool_reqbody_cl(p, r, p_conn, origin, header_brigade,
> input_brigade, (old_cl_val != NULL)
> || (old_te_val != NULL)
> @@ -947,6 +967,7 @@ skip_body:
>
> }
>
> if (rv != OK) {
> + conn_rec *c = r->connection;
> /* apr_status_t value has been logged in lower level method */
> ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01097)
> "pass request body failed to %pI (%s) from %s (%s)",
> @@ -1854,10 +1875,16 @@ static int proxy_http_handler(request_rec *r, prox
>
> char *scheme;
> const char *proxy_function;
> const char *u;
> + apr_bucket_brigade *header_brigade, *header_bb;
> + apr_bucket_brigade *input_brigade, *input_bb;
> proxy_conn_rec *backend = NULL;
> int is_ssl = 0;
> conn_rec *c = r->connection;
> int retry = 0;
> + char *old_cl_val = NULL, *old_te_val = NULL;
> + enum rb_methods rb_method = RB_INIT;
> + char *locurl = url;
> + int toclose = 0;
>
> /*
> * Use a shorter-lived pool to reduce memory usage
> * and avoid a memory leak
> @@ -1924,16 +1951,55 @@ static int proxy_http_handler(request_rec *r, prox
>
> backend->close = 1;
> }
>
> + /* Step One: Determine Who To Connect To */
> + if ((status = ap_proxy_determine_connection(p, r, conf, worker,
> backend,
> + uri, &locurl, proxyname,
> + proxyport, server_portstr,
> + sizeof(server_portstr))) !=
> OK)
> + goto cleanup;
> +
> + /* Step Once: Prefetch (partially) the request body so to increase the
> + * chances to get whole (or enough) body and determine Content-Length
> vs
> + * chunked or spool. By doing this before connecting or reusing a
> backend
> + * connection, minimize the delay between checking whether this
> connection
> + * is still alive and the first packet sent, should the client be
> slow or
> + * some input filter retain the data.
> + */
> + input_brigade = apr_brigade_create(p, c->bucket_alloc);
> + header_brigade = apr_brigade_create(p, c->bucket_alloc);
> + if ((status = ap_proxy_http_prefetch(p, r, backend, worker, conf, uri,
> + locurl, server_portstr, header_brigade, input_brigade,
> + &old_cl_val, &old_te_val, &rb_method)) != OK) {
>
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO()
> + "HTTP: failed to prefetch the request body: %s",
> + backend->hostname);
> + goto cleanup;
> + }
> +
> + /* XXX: Reset backend->close now, since ap_proxy_http_prefetch sets
> it to
> + * disable the reuse of the connection after this request (no
> keep-alive),
> + * not to close any reusable connection before this request. However
> assure
> + * what is expected later by using a local flag and do the right
> thing when
> + * ap_proxy_connect_backend (below) provides the connection to close.
> + */
> + toclose = backend->close;
> + backend->close = 0;
>
> +
> while (retry < 2) {
> - char *locurl = url;
> + if (retry) {
> + char *newurl = url;
> + /* Step One-Retry: Redetermine Who To Connect To */
> + if ((status = ap_proxy_determine_connection(p, r, conf,
> worker,
> + backend, uri, &newurl, proxyname, proxyport,
> + server_portstr, sizeof(server_portstr))) !=
> OK)
> + break;
> + /* XXX: the code assumes locurl is not changed during the
> loops,
> + * or ap_proxy_http_prefetch would have to be called every
> time,
> + * and header_brigade changed accordingly...
> + */
> + AP_DEBUG_ASSERT(!strcmp(newurl, locurl));
> + }
>
> - /* Step One: Determine Who To Connect To */
> - if ((status = ap_proxy_determine_connection(p, r, conf, worker,
> backend,
> - uri, &locurl, proxyname,
> - proxyport, server_portstr,
> - sizeof(server_portstr)))
> != OK)
> - break;
> -
> /* Step Two: Make the Connection */
> if (ap_proxy_connect_backend(proxy_function, backend, worker,
> r->server)) {
> ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01114)
> @@ -1975,7 +2041,19 @@ static int proxy_http_handler(request_rec *r, prox
> }
> }
>
> + /* Don't recycle the connection if prefetch (above) told not to
> do so */
> + if (toclose) {
>
> + backend->close = 1;
> + backend->connection->keepalive = AP_CONN_CLOSE;
> + }
>
> +
> /* Step Three-and-a-Half: See if the socket is still connected
> (if desired) */
> + /* XXX: ap_proxy_connect_backend aleady unconditionally checked
> that
> + * before, if ap_proxy_connection_create was not called in the
> meantime
> + * this check is redundant, otherwise can
> ap_proxy_connection_create
> + * take so much time that a new check is relevant here? At least
> this
> + * code should be run only when ap_proxy_connection_create is
> called.
> + */
>
> if (worker->s->ping_timeout_set && worker->s->ping_timeout < 0 &&
> !ap_proxy_is_socket_connected(backend->sock)) {
> backend->close = 1;
> @@ -1986,12 +2064,18 @@ static int proxy_http_handler(request_rec *r, prox
> continue;
> }
>
> + /* Preserve the header/input brigades since they may be retried.
> */
>
> + input_bb = apr_brigade_create(p,
> backend->connection->bucket_alloc);
> + header_bb = apr_brigade_create(p,
> backend->connection->bucket_alloc);
> + proxy_buckets_lifetime_transform(r, input_brigade, input_bb);
> + proxy_buckets_lifetime_transform(r, header_brigade, header_bb);
> +
> /* Step Four: Send the Request
> * On the off-chance that we forced a 100-Continue as a
> * kinda HTTP ping test, allow for retries
> */
> - if ((status = ap_proxy_http_request(p, r, backend, worker,
> - conf, uri, locurl,
> server_portstr)) != OK) {
> + if ((status = ap_proxy_http_request(p, r, backend, header_bb,
> input_bb,
> + old_cl_val, old_te_val, rb_method)) != OK) {
> if ((status == HTTP_SERVICE_UNAVAILABLE) &&
> worker->s->ping_timeout_set &&
> worker->s->ping_timeout > 0) {
> backend->close = 1;
> Index: modules/proxy/proxy_util.c
> ===================================================================
> --- modules/proxy/proxy_util.c (revision 1529127)
>
> +++ modules/proxy/proxy_util.c (working copy)
> @@ -3092,7 +3092,9 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
> buf = apr_pstrcat(p, r->method, " ", url, " HTTP/1.1" CRLF, NULL);
> }
> if (apr_table_get(r->subprocess_env, "proxy-nokeepalive")) {
> - origin->keepalive = AP_CONN_CLOSE;
> + if (origin) {
> + origin->keepalive = AP_CONN_CLOSE;
> + }
> p_conn->close = 1;
> }
> ap_xlate_proto_to_ascii(buf, strlen(buf));
> [EOS]
>
