Hi Luca,
On Wed, Jun 6, 2018 at 12:35 PM, Luca Toscano <toscano.l...@gmail.com> wrote:
>
> Tried to improve the ctx->tmpbb usage adding a new ctx->buffer brigade, with
> the only duty of buffering buckets between filter's invocations (if needed):
>
> http://home.apache.org/~elukey/httpd-trunk-mod_ratelimit-rate_limit_proxied_content.patch
>
> I didn't touch holdingbb since it is already used to allow buckets to skip
> the rate limit and go full speed, even if I am not sure if this
> functionality has it ever been used. I would prefer not to touch that
> special logic and fix only this use case.
We possibly don't need another brigade, and can save the remaining
buckets at the end only (not far from your patch, see attached).
This patch (over yours) uses apr_brigade_split_ex() instead of
open-coding it, and returns errors immediately (the error logic was
buggy and this makes the code simpler).
Also as you said, all the RATE_FULLSPEED/RATE_LIMIT logic seems to be
dead code, I think we can axe it (at least in trunk) as follow up.
This would remove rl_buckets and complexity in the loop, but for 2.4.x
we probably want to keep it since it's public API (any third party
module using these buckets?).
Feel free to take what can serve you, or burn it ;)
Anyway, thanks for working on this!
Index: modules/filters/mod_ratelimit.c
===================================================================
--- modules/filters/mod_ratelimit.c (revision 1833084)
+++ modules/filters/mod_ratelimit.c (working copy)
@@ -26,7 +26,6 @@
typedef enum rl_state_e
{
- RATE_ERROR,
RATE_LIMIT,
RATE_FULLSPEED
} rl_state_e;
@@ -36,6 +35,7 @@ typedef struct rl_ctx_t
int speed;
int chunk_size;
int burst;
+ int do_sleep;
rl_state_e state;
apr_bucket_brigade *tmpbb;
apr_bucket_brigade *holdingbb;
@@ -57,21 +57,12 @@ static void brigade_dump(request_rec *r, apr_bucke
#endif /* RLFDEBUG */
static apr_status_t
-rate_limit_filter(ap_filter_t *f, apr_bucket_brigade *input_bb)
+rate_limit_filter(ap_filter_t *f, apr_bucket_brigade *bb)
{
apr_status_t rv = APR_SUCCESS;
rl_ctx_t *ctx = f->ctx;
- apr_bucket *fb;
- int do_sleep = 0;
apr_bucket_alloc_t *ba = f->r->connection->bucket_alloc;
- apr_bucket_brigade *bb = input_bb;
- if (f->c->aborted) {
- ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, f->r, APLOGNO(01454) "rl: conn aborted");
- apr_brigade_cleanup(bb);
- return APR_ECONNABORTED;
- }
-
/* Set up our rl_ctx_t on first use */
if (ctx == NULL) {
@@ -120,6 +111,7 @@ static apr_status_t
ctx->state = RATE_LIMIT;
ctx->speed = ratelimit;
ctx->burst = burst;
+ ctx->do_sleep = 0;
/* calculate how many bytes / interval we want to send */
/* speed is bytes / second, so, how many (speed / 1000 % interval) */
@@ -127,56 +119,40 @@ static apr_status_t
ctx->tmpbb = apr_brigade_create(f->r->pool, ba);
ctx->holdingbb = apr_brigade_create(f->r->pool, ba);
}
+ else {
+ APR_BRIGADE_PREPEND(bb, ctx->holdingbb);
+ }
- while (ctx->state != RATE_ERROR &&
- (!APR_BRIGADE_EMPTY(bb) || !APR_BRIGADE_EMPTY(ctx->holdingbb))) {
+ while (!APR_BRIGADE_EMPTY(bb)) {
apr_bucket *e;
- if (!APR_BRIGADE_EMPTY(ctx->holdingbb)) {
- APR_BRIGADE_CONCAT(bb, ctx->holdingbb);
- }
-
- while (ctx->state == RATE_FULLSPEED && !APR_BRIGADE_EMPTY(bb)) {
+ if (ctx->state == RATE_FULLSPEED) {
/* Find where we 'stop' going full speed. */
for (e = APR_BRIGADE_FIRST(bb);
e != APR_BRIGADE_SENTINEL(bb); e = APR_BUCKET_NEXT(e)) {
if (AP_RL_BUCKET_IS_END(e)) {
- apr_bucket *f;
- f = APR_RING_LAST(&bb->list);
- APR_RING_UNSPLICE(e, f, link);
- APR_RING_SPLICE_TAIL(&ctx->holdingbb->list, e, f,
- apr_bucket, link);
+ apr_brigade_split_ex(bb, e, ctx->holdingbb);
ctx->state = RATE_LIMIT;
break;
}
}
- if (f->c->aborted) {
- apr_brigade_cleanup(bb);
- ctx->state = RATE_ERROR;
- break;
- }
-
- fb = apr_bucket_flush_create(ba);
- APR_BRIGADE_INSERT_TAIL(bb, fb);
+ e = apr_bucket_flush_create(ba);
+ APR_BRIGADE_INSERT_TAIL(bb, e);
rv = ap_pass_brigade(f->next, bb);
+ apr_brigade_cleanup(bb);
if (rv != APR_SUCCESS) {
- ctx->state = RATE_ERROR;
ap_log_rerror(APLOG_MARK, APLOG_TRACE1, rv, f->r, APLOGNO(01455)
"rl: full speed brigade pass failed.");
+ return rv;
}
}
-
- while (ctx->state == RATE_LIMIT && !APR_BRIGADE_EMPTY(bb)) {
+ else {
for (e = APR_BRIGADE_FIRST(bb);
e != APR_BRIGADE_SENTINEL(bb); e = APR_BUCKET_NEXT(e)) {
if (AP_RL_BUCKET_IS_START(e)) {
- apr_bucket *f;
- f = APR_RING_LAST(&bb->list);
- APR_RING_UNSPLICE(e, f, link);
- APR_RING_SPLICE_TAIL(&ctx->holdingbb->list, e, f,
- apr_bucket, link);
+ apr_brigade_split_ex(bb, e, ctx->holdingbb);
ctx->state = RATE_FULLSPEED;
break;
}
@@ -183,24 +159,8 @@ static apr_status_t
}
while (!APR_BRIGADE_EMPTY(bb)) {
- apr_bucket *stop_point;
- apr_off_t len = 0;
+ apr_off_t len = ctx->chunk_size + ctx->burst;
- if (f->c->aborted) {
- apr_brigade_cleanup(bb);
- ctx->state = RATE_ERROR;
- break;
- }
-
- if (do_sleep) {
- apr_sleep(RATE_INTERVAL_MS * 1000);
- }
- else {
- do_sleep = 1;
- }
-
- apr_brigade_length(bb, 1, &len);
-
/*
* Pull next chunk of data; the initial amount is our
* burst allotment (if any) plus a chunk. All subsequent
@@ -208,30 +168,53 @@ static apr_status_t
* burst amounts we have left (in case not done in the
* first bucket).
*/
- rv = apr_brigade_partition(bb,
- ctx->chunk_size + ctx->burst, &stop_point);
+ rv = apr_brigade_partition(bb, len, &e);
if (rv != APR_SUCCESS && rv != APR_INCOMPLETE) {
- ctx->state = RATE_ERROR;
ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, f->r, APLOGNO(01456)
"rl: partition failed.");
- break;
+ return rv;
}
-
- if (stop_point != APR_BRIGADE_SENTINEL(bb)) {
- apr_bucket *f;
- apr_bucket *e = APR_BUCKET_PREV(stop_point);
- f = APR_RING_FIRST(&bb->list);
- APR_RING_UNSPLICE(f, e, link);
- APR_RING_SPLICE_HEAD(&ctx->tmpbb->list, f, e, apr_bucket,
- link);
+ while (e != APR_BRIGADE_SENTINEL(bb)
+ && APR_BUCKET_IS_METADATA(e)) {
+ e = APR_BUCKET_NEXT(e);
}
+ if (e != APR_BRIGADE_SENTINEL(bb)) {
+ apr_brigade_split_ex(bb, e, ctx->tmpbb);
+ }
else {
- APR_BRIGADE_CONCAT(ctx->tmpbb, bb);
+ apr_brigade_length(bb, 1, &len);
}
+#if defined(RLFDEBUG)
+ brigade_dump(f->r, bb);
+ brigade_dump(f->r, ctx->tmpbb);
+#endif /* RLFDEBUG */
- fb = apr_bucket_flush_create(ba);
+ e = APR_BRIGADE_LAST(bb);
+ if (APR_BUCKET_IS_EOS(e)
+ || len >= ctx->chunk_size + ctx->burst) {
+ if (APR_BUCKET_IS_EOS(e)) {
+ ap_remove_output_filter(f);
+ }
+ else if (!APR_BUCKET_IS_FLUSH(e)) {
+ e = apr_bucket_flush_create(ba);
+ APR_BRIGADE_INSERT_TAIL(bb, e);
+ }
+ if (ctx->do_sleep) {
+ apr_sleep(RATE_INTERVAL_MS * 1000);
+ }
+ else {
+ ctx->do_sleep = 1;
+ }
+ rv = ap_pass_brigade(f->next, bb);
+ apr_brigade_cleanup(bb);
- APR_BRIGADE_INSERT_TAIL(ctx->tmpbb, fb);
+ if (rv != APR_SUCCESS) {
+ /* Most often, user disconnects from stream */
+ ap_log_rerror(APLOG_MARK, APLOG_TRACE1, rv, f->r, APLOGNO(01457)
+ "rl: brigade pass failed.");
+ return rv;
+ }
+ }
/*
* Adjust the burst amount depending on how much
@@ -238,8 +221,6 @@ static apr_status_t
* we've done up to now.
*/
if (ctx->burst) {
- len = ctx->burst;
- apr_brigade_length(ctx->tmpbb, 1, &len);
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, f->r,
APLOGNO(03485) "rl: burst %d; len %"APR_OFF_T_FMT, ctx->burst, len);
if (len < ctx->burst) {
@@ -249,27 +230,16 @@ static apr_status_t
ctx->burst = 0;
}
}
-
-#if defined(RLFDEBUG)
- brigade_dump(f->r, ctx->tmpbb);
- brigade_dump(f->r, bb);
-#endif /* RLFDEBUG */
-
- rv = ap_pass_brigade(f->next, ctx->tmpbb);
- apr_brigade_cleanup(ctx->tmpbb);
-
- if (rv != APR_SUCCESS) {
- /* Most often, user disconnects from stream */
- ctx->state = RATE_ERROR;
- ap_log_rerror(APLOG_MARK, APLOG_TRACE1, rv, f->r, APLOGNO(01457)
- "rl: brigade pass failed.");
- break;
- }
}
}
+
+ if (!APR_BRIGADE_EMPTY(ctx->holdingbb)) {
+ APR_BRIGADE_CONCAT(bb, ctx->holdingbb);
+ APR_BRIGADE_CONCAT(bb, ctx->tmpbb);
+ }
}
- return rv;
+ return ap_save_brigade(f, &ctx->holdingbb, &ctx->tmpbb, f->r->pool);
}
