Is the "Step Three-and-a-Half" really required since
ap_proxy_connect_backend() (that does the same is_socket_connected() check)
is run almost before? May the ap_proxy_connection_create() function in
between take some time or is it a last chance to catch the race?
Regards
On Thu, Oct 3, 2013 at 12:38 AM, Yann Ylavic <ylavic....@gmail.com> wrote:
> A late (little) fix below...
>
>
> On Thu, Oct 3, 2013 at 12:14 AM, Yann Ylavic <ylavic....@gmail.com> 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]
>>
>>
>
