httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ruediger Pluem <rpl...@apache.org>
Subject Re: svn commit: r921526 - /httpd/httpd/trunk/modules/filters/mod_reqtimeout.c
Date Fri, 12 Mar 2010 06:04:17 GMT
On 10.03.2010 20:40, sf@apache.org wrote:
> Author: sf
> Date: Wed Mar 10 19:40:51 2010
> New Revision: 921526
> 
> URL: http://svn.apache.org/viewvc?rev=921526&view=rev
> Log:
> - Enforce request timeout even for AP_MODE_GETLINE.
> - Abort connection when timeout occurs. Otherwise the thread may be blocked
>   another 30s doing a lingering close. The downside is that the client will
>   not receive the error message.
> 
> Modified:
>     httpd/httpd/trunk/modules/filters/mod_reqtimeout.c
> 
> Modified: httpd/httpd/trunk/modules/filters/mod_reqtimeout.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/filters/mod_reqtimeout.c?rev=921526&r1=921525&r2=921526&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/filters/mod_reqtimeout.c (original)
> +++ httpd/httpd/trunk/modules/filters/mod_reqtimeout.c Wed Mar 10 19:40:51 2010
> @@ -24,6 +24,7 @@

> @@ -139,34 +181,98 @@ static apr_status_t reqtimeout_filter(ap
>          return rv;
>      }
>  
> -    if (time_left < apr_time_from_sec(1)) {
> -        time_left = apr_time_from_sec(1);
> -    }
> -
>      rv = apr_socket_timeout_get(ccfg->socket, &saved_sock_timeout);
>      AP_DEBUG_ASSERT(rv == APR_SUCCESS);
>  
> -    if (saved_sock_timeout >= time_left) {
> -        rv = apr_socket_timeout_set(ccfg->socket, time_left);
> -        AP_DEBUG_ASSERT(rv == APR_SUCCESS);
> -    }
> -    else {
> -        saved_sock_timeout = -1;
> -    }
> +    rv = apr_socket_timeout_set(ccfg->socket, MIN(time_left, saved_sock_timeout));
> +    AP_DEBUG_ASSERT(rv == APR_SUCCESS);
>  
> -    rv = ap_get_brigade(f->next, bb, mode, block, readbytes);
> +    if (mode == AP_MODE_GETLINE) {
> +        /*
> +         * For a blocking AP_MODE_GETLINE read, apr_brigade_split_line()
> +         * would loop until a whole line has been read. As this would make it
> +         * impossible to enforce a total timeout, we only do non-blocking
> +         * reads.
> +         */
> +        apr_size_t remaining = HUGE_STRING_LEN;
> +        do {
> +            apr_off_t bblen;
> +
> +            rv = ap_get_brigade(f->next, bb, AP_MODE_GETLINE, APR_NONBLOCK_READ,
remaining);
> +            if (APR_STATUS_IS_EAGAIN(rv)) {
> +                rv = APR_SUCCESS;
> +            }
> +            else if (rv != APR_SUCCESS) {
> +                break;
> +            }
> +
> +            if (!APR_BRIGADE_EMPTY(bb)) {
> +                rv = have_lf_or_eos(bb);
> +                if (rv != APR_INCOMPLETE) {
> +                    break;
> +                }
> +
> +                if (ccfg->min_rate > 0) {
> +                    extend_timeout(ccfg, bb);
> +                }

Hm, shouldn't we extend the timeout in the case of APR_SUCCESS as well?
We likely have future reads of headers in this case.

> +
> +                rv = apr_brigade_length(bb, 1, &bblen);
> +                if (rv != APR_SUCCESS) {
> +                    break;
> +                }
> +                remaining -= bblen;
> +                if (remaining <= 0) {
> +                    break;
> +                }
> +
> +                /* Haven't got a whole line yet, save what we have ... */
> +                if (!ccfg->tmpbb) {
> +                    ccfg->tmpbb = apr_brigade_create(f->c->pool, f->c->bucket_alloc);
> +                }
> +                APR_BRIGADE_CONCAT(ccfg->tmpbb, bb);
> +            }
> +
> +            /* ... and wait for more */
> +            rv = apr_wait_for_io_or_timeout(NULL, ccfg->socket, 1);
> +            if (rv != APR_SUCCESS)
> +                break;
> +
> +            rv = check_time_left(ccfg, &time_left);
> +            if (rv != APR_SUCCESS)
> +                break;
> +
> +            rv = apr_socket_timeout_set(ccfg->socket,
> +                                   MIN(time_left, saved_sock_timeout));
> +            AP_DEBUG_ASSERT(rv == APR_SUCCESS);
>  
> -    if (saved_sock_timeout != -1) {
> -        apr_socket_timeout_set(ccfg->socket, saved_sock_timeout);
> -    }
> +        } while (1);
> +
> +        if (ccfg->tmpbb)
> +            APR_BRIGADE_PREPEND(bb, ccfg->tmpbb);
>  
> -    if (ccfg->min_rate > 0 && rv == APR_SUCCESS) {
> -        extend_timeout(ccfg, bb);
>      }
> +    else {
> +        /* mode != AP_MODE_GETLINE */
> +        rv = ap_get_brigade(f->next, bb, mode, block, readbytes);
> +        if (ccfg->min_rate > 0 && rv == APR_SUCCESS) {
> +            extend_timeout(ccfg, bb);
> +        }
> +    }
> +
> +    apr_socket_timeout_set(ccfg->socket, saved_sock_timeout);
>  
> +out:
>      if (rv == APR_TIMEUP) {
>          ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, f->c,
>                        "Request %s read timeout", ccfg->type);
> +        /*
> +         * If we allow lingering close, the client may keep this
> +         * process/thread busy for another 30s (MAX_SECS_TO_LINGER).
> +         * Therefore we have to abort the connection. The downside is
> +         * that the client will most likely not receive the error
> +         * message.
> +         */
> +        f->c->aborted = 1;

We had previous discussions on list (don't have pointers at hand right now),
that c->aborted is not allowed to be used to abort a connection from our side
but that it is strictly reserved to indicate that the client aborted the
connection. On the other side I currently have no alternate proposal to get
the worthwhile effect you want by doing this. Maybe we need to create a new
API / flag on trunk for this?

Regards

RĂ¼diger


Mime
View raw message