A late (little) fix below...
On Thu, Oct 3, 2013 at 12:14 AM, Yann Ylavic <[email protected]> wrote: > > Index: modules/proxy/proxy_util.c > =================================================================== > --- modules/proxy/proxy_util.c (revision 1528615) > +++ 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)); > Index: modules/proxy/mod_proxy_http.c > =================================================================== > --- modules/proxy/mod_proxy_http.c (revision 1528615) > +++ modules/proxy/mod_proxy_http.c (working copy) > @@ -677,30 +677,41 @@ 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, int > *keepalive) > { > 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; > > + /* XXX: This somehow should be fixed in the API. > + * Assumes p_conn->close is 0 here since ap_proxy_create_hdrbrgd and > code > + * below will use it to disable keep-alive and not for the connection > to > + * be closed before reuse. > + */ > + AP_DEBUG_ASSERT(!p_conn->close); > > + > if (apr_table_get(r->subprocess_env, "force-proxy-request-1.0")) { > if (r->expecting_100) { > return HTTP_EXPECTATION_FAILED; > @@ -710,17 +721,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 +740,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 +762,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 +873,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 +930,28 @@ skip_body: > APR_BRIGADE_INSERT_TAIL(header_brigade, e); > } > > + /* XXX: This somehow should be fixed in the API (see beginning > comment). > + * backend->close is set to disable keepalive, no need to close the > socket > + * until further notice, let the caller know about that though. > + */ > + *keepalive = !p_conn->close; > + p_conn->close = 0; > + > + 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 +963,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 +976,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 +1884,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 keepalive = 1; > /* > * Use a shorter-lived pool to reduce memory usage > * and avoid a memory leak > @@ -1924,16 +1960,47 @@ 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, &keepalive)) != > OK) { > + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO() > + "HTTP: failed to prefetch the request body: %s", > + backend->hostname); > + goto cleanup; > + } > + > 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) > @@ -1974,6 +2041,9 @@ static int proxy_http_handler(request_rec *r, prox > ssl_hostname); > } > } > Move this block : > + if (!keepalive) { > + backend->connection->keepalive = AP_CONN_CLOSE; > including this addon to ensure closure when not kept alive : + backend->close = 1; > + } > after "Step Three-and-a-Half" below. > > /* Step Three-and-a-Half: See if the socket is still connected > (if desired) */ > if (worker->s->ping_timeout_set && worker->s->ping_timeout < 0 && > @@ -1986,12 +2056,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; > [EOS] > >
