You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Eric Covener <co...@gmail.com> on 2016/07/28 22:30:32 UTC
Re: svn commit: r1678698 - in /httpd/httpd/branches/2.2.x: ./ STATUS modules/filters/mod_reqtimeout.c
I think this backport isn't helping in 2.2.x. In 2.2
check_pipeline_flush() doesn't use AP_MODE_SPECULATIVE :(
I wonder what the risk is non-blocking mode-AP_MODE_EATCRLF to do the
same thing.
static void check_pipeline_flush(request_rec *r)
{
apr_bucket *e;
apr_bucket_brigade *bb;
conn_rec *c = r->connection;
/* ### if would be nice if we could PEEK without a brigade. that would
### allow us to defer creation of the brigade to when we actually
### need to send a FLUSH. */
bb = apr_brigade_create(r->pool, c->bucket_alloc);
/* Flush the filter contents if:
*
* 1) the connection will be closed
* 2) there isn't a request ready to be read
*/
/* ### shouldn't this read from the connection input filters? */
/* ### is zero correct? that means "read one line" */
if (r->connection->keepalive != AP_CONN_CLOSE) {
if (ap_get_brigade(r->input_filters, bb, AP_MODE_EATCRLF,
APR_NONBLOCK_READ, 0) != APR_SUCCESS) {
On Mon, May 11, 2015 at 5:07 AM, <yl...@apache.org> wrote:
> Author: ylavic
> Date: Mon May 11 09:07:56 2015
> New Revision: 1678698
>
> URL: http://svn.apache.org/r1678698
> Log:
> Merge r1621453 from trunk.
>
> PR56729: mod_reqtimeout gets confused when it sees a read via
> check_pipeline() just before a slow HTTP response completes.
>
> Committed By: coverner
> Reviewed By: covener, trawick, ylavic
> Backported By: ylavic
>
> Modified:
> httpd/httpd/branches/2.2.x/ (props changed)
> httpd/httpd/branches/2.2.x/STATUS
> httpd/httpd/branches/2.2.x/modules/filters/mod_reqtimeout.c
>
> Propchange: httpd/httpd/branches/2.2.x/
> ------------------------------------------------------------------------------
> --- svn:mergeinfo (original)
> +++ svn:mergeinfo Mon May 11 09:07:56 2015
> @@ -1,2 +1,2 @@
> /httpd/httpd/branches/2.4.x:1555538,1555559,1648845,1649003
> -/httpd/httpd/trunk:290940,395552,417988,451572,501364,583817,583830,611483,630858,639005,639010,647395,657354,657459,660461,660566,664330,678761,680082,681190,682369,683626,685112,686805,686809,687099,687754,693120,693392,693727-693728,696006,697093,706318,707163,708902,711421,713575,719357,720250,729316-729317,729586,732414,732504,732816,732832,733127,733134,733218-733219,734710,743589,755190,756671,756675,756678,756683,757741,761329,763394,764239,768535,769809,771587,771610,776325,777042,777091,778438-778439,778531,778942,780648,780655,780692,780697,780699,785457,785661,790587,803704,819480,823536,823563,834378,835046,891282,932791,942209,952823,953311,955966,992625,1026743,1031551,1040304,1040373,1058192,1070096,1082189,1082196,1090645,1172732,1222335,1223048,1231446,1244211,1294306,1299738,1300171,1301111,1308862,1327036,1327080,1328133,1328325-1328326,1345319,1348656,1349905,1352912,1363183,1363186,1366344,1367778,1368131,1368396,1369568,1395225,1398066,1400700,1408402,1410681,
> 1413732,1414094,1416889,1418752,1422234,1422253,1435178,1447426,1470940,1475878,1476604,1476621,1476642,1476644-1476645,1477530,1485409,1485668,1490994,1493330,1496429,1500323,1504276,1506714,1509872,1509875,1524192,1524770,1527925,1528718,1529559,1531505,1532816,1551714,1552227,1553204,1554276,1554281,1555240,1555555,1556428,1572092,1572198,1572543,1572611,1572630,1572655,1572663,1572668-1572671,1572896,1572911,1572967,1573224,1573229,1575400,1586745,1587594,1587639,1590509,1603156,1604353,1610207,1610311,1610491,1610501,1620932,1648840,1649001,1676654
> +/httpd/httpd/trunk:290940,395552,417988,451572,501364,583817,583830,611483,630858,639005,639010,647395,657354,657459,660461,660566,664330,678761,680082,681190,682369,683626,685112,686805,686809,687099,687754,693120,693392,693727-693728,696006,697093,706318,707163,708902,711421,713575,719357,720250,729316-729317,729586,732414,732504,732816,732832,733127,733134,733218-733219,734710,743589,755190,756671,756675,756678,756683,757741,761329,763394,764239,768535,769809,771587,771610,776325,777042,777091,778438-778439,778531,778942,780648,780655,780692,780697,780699,785457,785661,790587,803704,819480,823536,823563,834378,835046,891282,932791,942209,952823,953311,955966,992625,1026743,1031551,1040304,1040373,1058192,1070096,1082189,1082196,1090645,1172732,1222335,1223048,1231446,1244211,1294306,1299738,1300171,1301111,1308862,1327036,1327080,1328133,1328325-1328326,1345319,1348656,1349905,1352912,1363183,1363186,1366344,1367778,1368131,1368396,1369568,1395225,1398066,1400700,1408402,1410681,
> 1413732,1414094,1416889,1418752,1422234,1422253,1435178,1447426,1470940,1475878,1476604,1476621,1476642,1476644-1476645,1477530,1485409,1485668,1490994,1493330,1496429,1500323,1504276,1506714,1509872,1509875,1524192,1524770,1527925,1528718,1529559,1531505,1532816,1551714,1552227,1553204,1554276,1554281,1555240,1555555,1556428,1572092,1572198,1572543,1572611,1572630,1572655,1572663,1572668-1572671,1572896,1572911,1572967,1573224,1573229,1575400,1586745,1587594,1587639,1590509,1603156,1604353,1610207,1610311,1610491,1610501,1620932,1621453,1648840,1649001,1676654
>
> Modified: httpd/httpd/branches/2.2.x/STATUS
> URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.2.x/STATUS?rev=1678698&r1=1678697&r2=1678698&view=diff
> ==============================================================================
> --- httpd/httpd/branches/2.2.x/STATUS (original)
> +++ httpd/httpd/branches/2.2.x/STATUS Mon May 11 09:07:56 2015
> @@ -101,13 +101,6 @@ RELEASE SHOWSTOPPERS:
> PATCHES ACCEPTED TO BACKPORT FROM TRUNK:
> [ start all new proposals below, under PATCHES PROPOSED. ]
>
> - * mod_reqtimeout: PR56729: mod_reqtimeout gets confused when it sees a read
> - via check_pipeline() just before a slow HTTP response completes.
> - trunk patch: http://svn.apache.org/viewvc?view=revision&revision=r1621453
> - 2.2.x patch: http://people.apache.org/~covener/patches/httpd-2.2.2-reqtimeout-slow-post.diff
> - (minor merge conflict)
> - +1 covener, trawick, ylavic
> -
> * mod_deflate: Define APR_INT32_MAX when it is missing so to be able to
> compile against APR-1.2.x (minimum required version).
> trunk/2.4.x patch: not concerned (require APR-1.5.x)
>
> Modified: httpd/httpd/branches/2.2.x/modules/filters/mod_reqtimeout.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.2.x/modules/filters/mod_reqtimeout.c?rev=1678698&r1=1678697&r2=1678698&view=diff
> ==============================================================================
> --- httpd/httpd/branches/2.2.x/modules/filters/mod_reqtimeout.c (original)
> +++ httpd/httpd/branches/2.2.x/modules/filters/mod_reqtimeout.c Mon May 11 09:07:56 2015
> @@ -171,7 +171,18 @@ static apr_status_t reqtimeout_filter(ap
> 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);
> + }
> +
> now = apr_time_now();
> +
> if (ccfg->new_timeout > 0) {
> /* set new timeout */
> ccfg->timeout_at = now + apr_time_from_sec(ccfg->new_timeout);
>
>
--
Eric Covener
covener@gmail.com
Re: svn commit: r1678698 - in /httpd/httpd/branches/2.2.x: ./ STATUS modules/filters/mod_reqtimeout.c
Posted by Eric Covener <co...@gmail.com>.
> + if (apr_table_get(f->c->notes, "check-pipeline")) {
> /* 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
> + * during log_transaction().
> */
> return ap_get_brigade(f->next, bb, mode, block, readbytes);
I had a similar thought, will let it stew over the weekend.
Re: svn commit: r1678698 - in /httpd/httpd/branches/2.2.x: ./ STATUS modules/filters/mod_reqtimeout.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Jul 29, 2016 at 12:30 AM, Eric Covener <co...@gmail.com> wrote:
> I think this backport isn't helping in 2.2.x. In 2.2
> check_pipeline_flush() doesn't use AP_MODE_SPECULATIVE :(
>
> I wonder what the risk is non-blocking mode-AP_MODE_EATCRLF to do the
> same thing.
Maybe a safer way to handle this in 2.2 could be something like:
Index: modules/filters/mod_reqtimeout.c
===================================================================
--- modules/filters/mod_reqtimeout.c (revision 1754267)
+++ modules/filters/mod_reqtimeout.c (working copy)
@@ -171,12 +171,10 @@ static apr_status_t reqtimeout_filter(ap_filter_t
return ap_get_brigade(f->next, bb, mode, block, readbytes);
}
- if (block == APR_NONBLOCK_READ && mode == AP_MODE_SPECULATIVE) {
+ if (apr_table_get(f->c->notes, "check-pipeline")) {
/* 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
+ * during log_transaction().
*/
return ap_get_brigade(f->next, bb, mode, block, readbytes);
}
Index: modules/http/http_request.c
===================================================================
--- modules/http/http_request.c (revision 1754267)
+++ modules/http/http_request.c (working copy)
@@ -245,8 +245,12 @@ static void check_pipeline_flush(request_rec *r)
/* ### shouldn't this read from the connection input filters? */
/* ### is zero correct? that means "read one line" */
if (r->connection->keepalive != AP_CONN_CLOSE) {
- if (ap_get_brigade(r->input_filters, bb, AP_MODE_EATCRLF,
- APR_NONBLOCK_READ, 0) != APR_SUCCESS) {
+ apr_status_t rv;
+ apr_table_setn(c->notes, "check-pipeline", "1");
+ rv = ap_get_brigade(r->input_filters, bb, AP_MODE_EATCRLF,
+ APR_NONBLOCK_READ, 0);
+ apr_table_unset(c->notes, "check-pipeline");
+ if (rv != APR_SUCCESS) {
c->data_in_input_filters = 0; /* we got APR_EOF or an error */
}
else {
_