2018-06-08 12:43 GMT+02:00 Luca Toscano <toscano.l...@gmail.com>: > Hi Yann, > > 2018-06-07 12:59 GMT+02:00 Yann Ylavic <ylavic....@gmail.com>: > >> On Thu, Jun 7, 2018 at 10:17 AM, Yann Ylavic <ylavic....@gmail.com> >> wrote: >> > On Thu, Jun 7, 2018 at 9:21 AM, Yann Ylavic <ylavic....@gmail.com> >> wrote: >> >> >> >> Feel free to take what can serve you, or burn it ;) >> > >> > Burn it! Here is v2, hopefully a less buggy :) >> >> FWIW, a v3 with less changes and a few comments where needed. >> Less changes because it keeps using tmpbb in the RATE_LIMIT loop (with >> APR_BRIGADE_CONCAT(ctx->tmpbb, bb) first). >> HTH... >> > > All my consistency tests are green, and after a first (and quick!) pass of > the code in your patch I can definitely say that your version is much > better and coincise, I like it! Will try to review it in detail during the > next days and then I'll commit it. >
Sorry for the delay :) While reading the code I didn't get if one particular use case may or may not happen at all, it is the only big doubt that I have before committing. Is it possible that the first bb to process (during the first invocation of the filter) is equal to the chunk_size + ctx->burst? IIUC the code relies on the fact that bb is not empty to decide whether or not to pass ctx->tmpp to the next filter in the chain, but if this is not the case then it may loop? I tried to dump the brigades of my tests (proxying content via mod_proxy_http and directly from httpd), the response headers in the beginning seems to always guarantee an initial bb of some bytes that make everything work. Not sure if this is a valuable comment, the rest looks super good :) Thanks for the patience! Luca
