2018-06-05 8:19 GMT+02:00 Luca Toscano <[email protected]>: > > > 2018-06-04 16:22 GMT+02:00 Luca Toscano <[email protected]>: > >> Hi Yann! >> >> 2018-06-04 15:59 GMT+02:00 Yann Ylavic <[email protected]>: >> >>> Hi Luca, >>> >>> On Mon, Jun 4, 2018 at 11:23 AM, Luca Toscano <[email protected]> >>> wrote: >>> > >>> > To keep archives happy: opened >>> > https://bz.apache.org/bugzilla/show_bug.cgi?id=62362 and added a >>> patch in >>> > there, if anybody wants to review it and give me suggestions I'd be >>> happy :) >>> >>> The semantic of tmpbb is not very clear in your patch, it's both the >>> brigade where buckets are saved (for the next call?) and the one that >>> gets passed to the next filter finally. >>> It doesn't look right to me, if tmpbb is to be forwarded in the same >>> pass there is no need to change its buckets' lifetime. >>> Couldn't we ap_save_brigade(f, &ctx->holdingbb, &ctx->tmpbb, ..) at >>> the end of the filter only? >>> >> >> Thanks for the review, bare in mind that this is my first "big" patch so >> I'd probably need a better grasp of the internals first :) I haven't >> touched holdingbb since I saw that was used elsewhere (in RATE_FULLSPEED), >> but I can try to check it as well. The idea of my patch (that I aimed to) >> is to pass the ctx->tmbbb only if it reaches chunk_size (or EOS is seen) >> and buffer otherwise the buckets in it using ap_save_brigade (waiting for >> the next call to see if chunk_size is reached). >> >> So if I got your point correctly you would use ctx->holdingbb to store >> the buckets (and changing their lifetime possibly) between calls, and tmpbb >> only within the same filter invocation? >> >> > After re-reading the code and I think I got your point Yann, I'll try to > re-work my idea and create a new patch. Thanks! >
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. I may have misunderstood Yann's suggestions, if so sorry for the spam, but I'd be glad to get a bit more info about how a better patch should look like :) I promise documentation in the docs/developer section when the task is done! (I can add a "If even Luca did this, you can as well!" section in the docs :P) Luca
