On 10.03.2010 20:40, [email protected] 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
