You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2010/03/12 07:04:17 UTC
Re: svn commit: r921526 - /httpd/httpd/trunk/modules/filters/mod_reqtimeout.c
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
Re: svn commit: r921526 - /httpd/httpd/trunk/modules/filters/mod_reqtimeout.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 12.03.2010 21:07, Stefan Fritsch wrote:
> On Fri, 12 Mar 2010, Ruediger Pluem wrote:
>> On 10.03.2010 20:40, sf@apache.org wrote:
>>> + 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.
>
> True, thanks. Fixed in r922407.
>
>>> +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?
>
> I didn't know about that restriction of c->aborted.
>
> Maybe add a note to the conn_rec that we need a quick lingering close
> and shorten the max wait time in ap_lingering_close() from 30 to 2
> seconds if that note is set? Since this will only be used in error
> conditions, the relatively expensive setting/checking of a note should
> not be a problem here. One could also make the lingering close wait time
> configurable, but I don't think that is really necessary.
>
>
IMHO the idea with note sounds good. Other opinions?
Regards
Rüdiger
Re: svn commit: r921526 -
/httpd/httpd/trunk/modules/filters/mod_reqtimeout.c
Posted by Stefan Fritsch <sf...@sfritsch.de>.
On Fri, 12 Mar 2010, Ruediger Pluem wrote:
> On 10.03.2010 20:40, sf@apache.org wrote:
>> + 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.
True, thanks. Fixed in r922407.
>> +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?
I didn't know about that restriction of c->aborted.
Maybe add a note to the conn_rec that we need a quick lingering close and
shorten the max wait time in ap_lingering_close() from 30 to 2 seconds if
that note is set? Since this will only be used in error conditions, the
relatively expensive setting/checking of a note should not be a problem
here. One could also make the lingering close wait time configurable, but
I don't think that is really necessary.