vsthrottle: add return_token() function

[danielbeardsley]

The docs explain it pretty well. With this, you can now limit concurrency per bucket. Most servers are significantly contrained in concurrency and a rate limit is most useful when your requests all take the same amount of time / resources. When requests take very different amount of resources / time, it's hard to find a rate limit that is sensible.

Limiting concurrency instead will help you by prenting a trickle of heavy long requests from burning you out.

Closes varnish/libvmod-vsthrottle#8

[dridi]

I will do a formal review later, but at first glance it looks very good!

[dridi]

Please note that this doesn't change the fact that requests may still take significant time once we return from vcl_deliver, and there's no guarantee that what was costly for the backend was to produce the response headers!

[nigoroll]

Please note that this doesn't change the fact that requests may still take significant time once we return from vcl_deliver, and there's no guarantee that what was costly for the backend was to produce the response headers!

It depends. For beresp.do_stream = false we are done with the body. But yes, for beresp.do_stream = true we'd want something like sub vcl_deliver_done {} or deliver_body_now() in sub vcl_deliver {}

[dridi]

Thanks @nigoroll for your review. You are correct to nitpick on do_stream but it defaults to true and in today's web where latency is critical I wouldn't recommend turning it off except for carefully selected use cases.

I noticed the $ABI vrt too but before complaining about this one I need to check that the module doesn't indeed foray into strict ABI symbols. That would be surprising, but that requires a full review of the VMOD, not just this feature.

@danielbeardsley can you please update your patch with @nigoroll's suggestions and mention the limitation of limiting concurrent requests in the context of streaming?

[nigoroll]

@Dridi FTR I do fully agree

[danielbeardsley]

Thanks for the reviews! I'll fix up those things tonight.

[danielbeardsley]

mention the limitation of limiting concurrent requests in the context of streaming?

Fixed with 7afcc27120e484e224de9bd7c63eab4766a0aad9

Thanks for all the feedback!

I'm realizing now that there's no perfect tie-in point the request lifecycle that will capture the effect I want, though both of these are ok:

• vcl_recv / deliver: does ok but ends when the response is ready to be sent, not when it's done sending
  • If your backend is non-streaming this may be fine cause the headers and body will be sent together (and thus the backend is done doing work) and so long as varnish reads all the response body quickly
  • This will also count varnish cache hits in the concurrency even though they don't hit your backend
• vcl_backend_fetch / response: closer to true cost, but can't serve 429s to the client, only abandon the backend request (resulting in 503)

[dridi]

I agree with @nigoroll, this looks OK to merge now. I will take the liberty to squash the commits and push the result myself.

[dridi]

Merged in 99837dca12a2ca9a13d521dcf166184e8ef24553.