On Sun, Nov 23, 2014 at 12:11 AM, Eric Covener <[email protected]> wrote:
> On Thu, Nov 20, 2014 at 9:57 AM, Yann Ylavic <[email protected]> wrote:
>> On Wed, Nov 19, 2014 at 1:13 PM, Eric Covener <[email protected]> wrote:
>>> On Wed, Nov 19, 2014 at 4:47 AM, Yann Ylavic <[email protected]> wrote:
>>>> Errr, this is in 2.2.x/STATUS only (not 2.4.x).
>>>> Is it already proposed/backported to 2.4.x (I can't find the commit)?
>>>
>>> I diff'ed trunk and 2.4 and It seems to be absent.
>>>
>>> I don't have the best handle on this, but if we're about to go down
>>> into a blocking read, wouldn't we want to check the time left and
>>> reduce the timeout?
>>
>> Yes, good point.
>>
>> Maybe this way then?
>>
>> Index: modules/filters/mod_reqtimeout.c
>> ===================================================================
>> --- modules/filters/mod_reqtimeout.c (revision 1640032)
>> +++ modules/filters/mod_reqtimeout.c (working copy)
>> @@ -311,7 +311,12 @@ static apr_status_t reqtimeout_filter(ap_filter_t
>> else {
>> /* mode != AP_MODE_GETLINE */
>> rv = ap_get_brigade(f->next, bb, mode, block, readbytes);
>> - if (ccfg->min_rate > 0 && rv == APR_SUCCESS) {
>> + /* Don't extend the timeout in speculative mode, wait for
>> + * the real (relevant) bytes to be asked later, within the
>> + * currently alloted time.
>> + */
>> + if (ccfg->min_rate > 0 && rv == APR_SUCCESS
>> + && mode != AP_MODE_SPECULATIVE) {
>> extend_timeout(ccfg, bb);
>> }
>> }
>
> Looks good
Commited in r1641376.
However, I now think you were right with your original proposal to
bypass the filter based on EOS :p
I don't see any reason why we should not do the same for blocking and
nonblocking reads.
The call from check_pipeline() is an exception that shouldn't
interfere with the real calls from modules.
So the current code (including r1641376) is probably "good enough" for
2.2.x, but maybe we could make it better for trunk and 2.4.x.
A first proposal is straight forward from you original patch :
- bypass the filter when the request is over (base on EOS seen),
- check the timeout (but don't extend it) in speculative mode for both
blocking and nonblocking reads.
See httpd-trunk-reqtimeout_filter_eos.patch attached.
A second proposal would be to have an optional function from
mod_reqtimeout to (des)activate itself on demand.
Thus check_pipeline() can use it (if not NULL, ie. mod_reqtimeout
loaded) before/after the read to desactivate/reactivate the checks.
This is maybe more intrusive (requires a new
ap_init_request_processing() function is http_request.h) but is less
dependent on mod_reqtimeout seeing the EOS (and it could also be used
where currently mod_reqtimeout is forcibly removed from the chain).
See httpd-trunk-reqtimeout_set_inactive.patch attached.
WDYT?
Index: modules/filters/mod_reqtimeout.c
===================================================================
--- modules/filters/mod_reqtimeout.c (revision 1641376)
+++ modules/filters/mod_reqtimeout.c (working copy)
@@ -64,6 +64,7 @@ typedef struct
} reqtimeout_con_cfg;
static const char *const reqtimeout_filter_name = "reqtimeout";
+static const char *const reqtimeout_filter_eos_name = "reqtimeout_eos";
static int default_header_rate_factor;
static int default_body_rate_factor;
@@ -176,23 +177,13 @@ static apr_status_t reqtimeout_filter(ap_filter_t
apr_status_t rv;
apr_interval_time_t saved_sock_timeout = UNSET;
reqtimeout_con_cfg *ccfg = f->ctx;
+ int extendable;
if (ccfg->in_keep_alive) {
/* For this read, the normal keep-alive timeout must be used */
- ccfg->in_keep_alive = 0;
return ap_get_brigade(f->next, bb, mode, block, readbytes);
}
- if (block == APR_NONBLOCK_READ && mode == AP_MODE_SPECULATIVE) {
- /* The source of these above us in the core is check_pipeline(), which
- * is between requests but before this filter knows to reset timeouts
- * during log_transaction(). If they appear elsewhere, just don't
- * check or extend the time since they won't block and we'll see the
- * bytes again later
- */
- return ap_get_brigade(f->next, bb, mode, block, readbytes);
- }
-
if (ccfg->new_timeout > 0) {
/* set new timeout */
now = apr_time_now();
@@ -212,6 +203,12 @@ static apr_status_t reqtimeout_filter(ap_filter_t
ccfg->socket = ap_get_conn_socket(f->c);
}
+ /* Don't extend the timeout in speculative mode, wait for
+ * the real (relevant) bytes to be asked later, within the
+ * currently alloted time.
+ */
+ extendable = (ccfg->min_rate > 0 && mode != AP_MODE_SPECULATIVE);
+
rv = check_time_left(ccfg, &time_left, now);
if (rv != APR_SUCCESS)
goto out;
@@ -219,7 +216,7 @@ static apr_status_t reqtimeout_filter(ap_filter_t
if (block == APR_NONBLOCK_READ || mode == AP_MODE_INIT
|| mode == AP_MODE_EATCRLF) {
rv = ap_get_brigade(f->next, bb, mode, block, readbytes);
- if (ccfg->min_rate > 0 && rv == APR_SUCCESS) {
+ if (extendable && rv == APR_SUCCESS) {
extend_timeout(ccfg, bb);
}
return rv;
@@ -253,7 +250,7 @@ static apr_status_t reqtimeout_filter(ap_filter_t
}
if (!APR_BRIGADE_EMPTY(bb)) {
- if (ccfg->min_rate > 0) {
+ if (extendable) {
extend_timeout(ccfg, bb);
}
@@ -315,8 +312,7 @@ static apr_status_t reqtimeout_filter(ap_filter_t
* the real (relevant) bytes to be asked later, within the
* currently alloted time.
*/
- if (ccfg->min_rate > 0 && rv == APR_SUCCESS
- && mode != AP_MODE_SPECULATIVE) {
+ if (extendable && rv == APR_SUCCESS) {
extend_timeout(ccfg, bb);
}
}
@@ -345,6 +341,29 @@ out:
return rv;
}
+/* Don't let core's check_pipeline() and alike trigger a timeout in between
+ * requests, when the connection is kept alive the corresponding timeout
+ * applies (reqtimeout_filter() bypassed).
+ */
+static apr_status_t reqtimeout_filter_eos(ap_filter_t *f,
+ apr_bucket_brigade *bb,
+ ap_input_mode_t mode,
+ apr_read_type_e block,
+ apr_off_t readbytes)
+{
+ apr_status_t rv;
+ reqtimeout_con_cfg *ccfg = f->ctx;
+ rv = ap_get_brigade(f->next, bb, mode, block, readbytes);
+ if (rv != APR_SUCCESS) {
+ return rv;
+ }
+ if (!APR_BRIGADE_EMPTY(bb) &&
+ APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(bb))) {
+ ccfg->in_keep_alive = 1;
+ }
+ return APR_SUCCESS;
+}
+
static int reqtimeout_init(conn_rec *c)
{
reqtimeout_con_cfg *ccfg;
@@ -416,6 +435,7 @@ static int reqtimeout_after_headers(request_rec *r
ccfg->min_rate = MRT_DEFAULT_BODY_MIN_RATE;
ccfg->rate_factor = default_body_rate_factor;
}
+ ap_add_input_filter(reqtimeout_filter_eos_name, ccfg, r, r->connection);
return OK;
}
@@ -436,7 +456,6 @@ static int reqtimeout_after_body(request_rec *r)
ccfg->timeout_at = 0;
ccfg->max_timeout_at = 0;
- ccfg->in_keep_alive = 1;
ccfg->type = "header";
if (ccfg->new_timeout != UNSET) {
ccfg->new_timeout = cfg->header_timeout;
@@ -615,6 +634,9 @@ static void reqtimeout_hooks(apr_pool_t *pool)
ap_register_input_filter(reqtimeout_filter_name, reqtimeout_filter, NULL,
AP_FTYPE_CONNECTION + 8);
+ ap_register_input_filter(reqtimeout_filter_eos_name, reqtimeout_filter_eos, NULL,
+ AP_FTYPE_CONTENT_SET);
+
/*
* mod_reqtimeout needs to be called before ap_process_http_request (which
* is run at APR_HOOK_REALLY_LAST) but after all other protocol modules.
Index: include/http_request.h
===================================================================
--- include/http_request.h (revision 1641373)
+++ include/http_request.h (working copy)
@@ -58,6 +58,11 @@ extern "C" {
#define AP_SUBREQ_MERGE_ARGS 1
/**
+ * Initialize request processing core.
+ */
+AP_DECLARE(int) ap_init_request_processing(apr_pool_t *p);
+
+/**
* An internal handler used by the ap_process_request, all subrequest mechanisms
* and the redirect mechanism.
* @param r The request, subrequest or internal redirect to pre-process
@@ -254,6 +259,11 @@ APR_DECLARE_OPTIONAL_FN(apr_array_header_t *, auth
APR_DECLARE_OPTIONAL_FN(apr_array_header_t *, authz_ap_list_provider_names,
(apr_pool_t *ptemp));
+/* Optional function coming from mod_reqtimeout to (des)activate itself
+ * temporarily (eg. during check_pipeline()).
+ */
+APR_DECLARE_OPTIONAL_FN(void, reqtimeout_set_inactive, (conn_rec *c, int to));
+
/**
* Determine if the current request is the main request or a subrequest
* @param r The current request
Index: modules/http/http_core.c
===================================================================
--- modules/http/http_core.c (revision 1641373)
+++ modules/http/http_core.c (working copy)
@@ -266,6 +266,8 @@ static int http_post_config(apr_pool_t *p, apr_poo
ap_random_insecure_bytes(&val, sizeof(val));
ap_multipart_boundary = apr_psprintf(p, "%0" APR_UINT64_T_HEX_FMT, val);
+ ap_init_request_processing(p);
+
return OK;
}
Index: modules/http/http_request.c
===================================================================
--- modules/http/http_request.c (revision 1641373)
+++ modules/http/http_request.c (working copy)
@@ -56,6 +56,14 @@ APLOG_USE_MODULE(http);
* Mainline request processing...
*/
+static APR_OPTIONAL_FN_TYPE(reqtimeout_set_inactive) *reqtimeout_set_inactive;
+
+AP_DECLARE(int) ap_init_request_processing(apr_pool_t *p)
+{
+ reqtimeout_set_inactive = APR_RETRIEVE_OPTIONAL_FN(reqtimeout_set_inactive);
+ return OK;
+}
+
/* XXX A cleaner and faster way to do this might be to pass the request_rec
* down the filter chain as a parameter. It would need to change for
* subrequest vs. main request filters; perhaps the subrequest filter could
@@ -222,8 +230,14 @@ static void check_pipeline(conn_rec *c)
apr_status_t rv;
apr_bucket_brigade *bb = apr_brigade_create(c->pool, c->bucket_alloc);
+ if (reqtimeout_set_inactive) {
+ reqtimeout_set_inactive(c, 1);
+ }
rv = ap_get_brigade(c->input_filters, bb, AP_MODE_SPECULATIVE,
APR_NONBLOCK_READ, 1);
+ if (reqtimeout_set_inactive) {
+ reqtimeout_set_inactive(c, 0);
+ }
if (rv != APR_SUCCESS || APR_BRIGADE_EMPTY(bb)) {
/*
* Error or empty brigade: There is no data present in the input
Index: modules/filters/mod_reqtimeout.c
===================================================================
--- modules/filters/mod_reqtimeout.c (revision 1641376)
+++ modules/filters/mod_reqtimeout.c (working copy)
@@ -56,7 +56,7 @@ typedef struct
int min_rate;
int new_timeout;
int new_max_timeout;
- int in_keep_alive;
+ int inactive;
char *type;
apr_socket_t *socket;
apr_time_t rate_factor;
@@ -176,23 +176,18 @@ static apr_status_t reqtimeout_filter(ap_filter_t
apr_status_t rv;
apr_interval_time_t saved_sock_timeout = UNSET;
reqtimeout_con_cfg *ccfg = f->ctx;
+ int extendable;
- if (ccfg->in_keep_alive) {
- /* For this read, the normal keep-alive timeout must be used */
- ccfg->in_keep_alive = 0;
+ if (ccfg->inactive) {
+ if (ccfg->inactive < 0) {
+ /* For this read only, the normal keep-alive timeout
+ * must be used.
+ */
+ ccfg->inactive = 0;
+ }
return ap_get_brigade(f->next, bb, mode, block, readbytes);
}
- if (block == APR_NONBLOCK_READ && mode == AP_MODE_SPECULATIVE) {
- /* The source of these above us in the core is check_pipeline(), which
- * is between requests but before this filter knows to reset timeouts
- * during log_transaction(). If they appear elsewhere, just don't
- * check or extend the time since they won't block and we'll see the
- * bytes again later
- */
- return ap_get_brigade(f->next, bb, mode, block, readbytes);
- }
-
if (ccfg->new_timeout > 0) {
/* set new timeout */
now = apr_time_now();
@@ -212,6 +207,12 @@ static apr_status_t reqtimeout_filter(ap_filter_t
ccfg->socket = ap_get_conn_socket(f->c);
}
+ /* Don't extend the timeout in speculative mode, wait for
+ * the real (relevant) bytes to be asked later, within the
+ * currently alloted time.
+ */
+ extendable = (ccfg->min_rate > 0 && mode != AP_MODE_SPECULATIVE);
+
rv = check_time_left(ccfg, &time_left, now);
if (rv != APR_SUCCESS)
goto out;
@@ -219,7 +220,7 @@ static apr_status_t reqtimeout_filter(ap_filter_t
if (block == APR_NONBLOCK_READ || mode == AP_MODE_INIT
|| mode == AP_MODE_EATCRLF) {
rv = ap_get_brigade(f->next, bb, mode, block, readbytes);
- if (ccfg->min_rate > 0 && rv == APR_SUCCESS) {
+ if (extendable && rv == APR_SUCCESS) {
extend_timeout(ccfg, bb);
}
return rv;
@@ -253,7 +254,7 @@ static apr_status_t reqtimeout_filter(ap_filter_t
}
if (!APR_BRIGADE_EMPTY(bb)) {
- if (ccfg->min_rate > 0) {
+ if (extendable) {
extend_timeout(ccfg, bb);
}
@@ -311,12 +312,7 @@ static apr_status_t reqtimeout_filter(ap_filter_t
else {
/* mode != AP_MODE_GETLINE */
rv = ap_get_brigade(f->next, bb, mode, block, readbytes);
- /* Don't extend the timeout in speculative mode, wait for
- * the real (relevant) bytes to be asked later, within the
- * currently alloted time.
- */
- if (ccfg->min_rate > 0 && rv == APR_SUCCESS
- && mode != AP_MODE_SPECULATIVE) {
+ if (extendable && rv == APR_SUCCESS) {
extend_timeout(ccfg, bb);
}
}
@@ -436,7 +432,7 @@ static int reqtimeout_after_body(request_rec *r)
ccfg->timeout_at = 0;
ccfg->max_timeout_at = 0;
- ccfg->in_keep_alive = 1;
+ ccfg->inactive = -1; /* set inactive for next keepalive read */
ccfg->type = "header";
if (ccfg->new_timeout != UNSET) {
ccfg->new_timeout = cfg->header_timeout;
@@ -605,6 +601,15 @@ static const char *set_reqtimeouts(cmd_parms *cmd,
}
+static void reqtimeout_set_inactive(conn_rec *c, int to)
+{
+ reqtimeout_con_cfg *ccfg =
+ ap_get_module_config(c->conn_config, &reqtimeout_module);
+ if (ccfg && ccfg->inactive == !to) {
+ ccfg->inactive = !!to;
+ }
+}
+
static void reqtimeout_hooks(apr_pool_t *pool)
{
/*
@@ -635,6 +640,8 @@ static void reqtimeout_hooks(apr_pool_t *pool)
#if MRT_DEFAULT_BODY_MIN_RATE > 0
default_body_rate_factor = apr_time_from_sec(1) / MRT_DEFAULT_BODY_MIN_RATE;
#endif
+
+ APR_REGISTER_OPTIONAL_FN(reqtimeout_set_inactive);
}
static const command_rec reqtimeout_cmds[] = {
