expire: Force a cache MISS when req.ttl = 0s

[AlveElde]

This commit changes the undefined behavior of setting req.ttl = 0s. Setting req.ttl = 0s would previously cause req.ttl to have no effect, but this breaks with setting req.grace = 0s, which does have an effect.

The use case is to achieve hash_always_miss semantics but with coalescing (waitinglist) (edit @nigoroll during bugwash)

The new behavior of setting req.ttl = 0s is similar to setting req.hash_always_miss = true, the main difference is that req.ttl allows for request coalescing. Useful for short-lived non-private objects.

I would like to define this behavior in the documentation somewhere, but didn't find a good place to put it.

Both req.ttl and req.grace are initialized to -1, which is why this change does not break everything. But setting req.ttl/grace to -1s in VCL ends up setting them to 0s, as these two req attributes have extra logic in their respective definitions: https://github.com/varnishcache/varnish-cache/blob/5f67d3aee3a756beb46bae610e1a75548e03c07f/bin/varnishd/cache/cache_vrt_var.c#L566-L569

Since this commit effectively prevents VCL writers from "clearing" req.ttl by setting it to 0s, maybe we should consider allowing users to clear req.ttl/grace by setting them to -1s?

[dridi]

Since this commit effectively prevents VCL writers from "clearing" req.ttl by setting it to 0s, maybe we should consider allowing users to clear req.ttl/grace by setting them to -1s?

Maybe this is for the best? What about this instead?

unset req.ttl;
unset req.grace;

[nigoroll]

My take: Integrate Dridis suggestion and update the docs, then it's a good improvement!

[dridi]

A thought occurred to me, shouldn't we use NAN when req.{ttl,grace} are unset?

[hermunn]

The problem with this PR is that it does not do what the title says. In the event of a waiting list, and there is a queue of clients waiting for an object, a newly inserted object will be "fresh", even if the "waiting list clients" all have req.ttl = 0s; and req.grace = 0s;, so these clients will get hits on the inserted object (the one which the clients on the waiting list were waiting for). This should be documented in a test case. Of course, this might be desired or undesired for the user, which is a problem.

The question is if we should special case the situation req.ttl = 0s; and req.grace = 0s; to mean I don't want a hit, grace or not, but do go on the waiting list if there is a busy object, and do grab the stale_oc if you find somthing. I don't like the idea of adding a new symbol for this behavior, in the way we have hash_always_miss and hash_ignore_busy, but it might be the "most correct" solution.

[dridi]

I think it would be more accurate to say "consider all objects stale when req.ttl = 0s".

Aren't we creating a perpetual waiter if the client disregards both TTL and grace? That is, when there is a busy object, the outcome will always be to disembark the request. So until the request wins the miss race, it will compete with other "req.ttl = req.grace = 0s" requests.

This is waiting list serialization once again, except that we swapped beresp with req to trigger it. I can try combining this change with #4032 to check whether it would eventually be mitigated.

[dridi]

Aren't we creating a perpetual waiter if the client disregards both TTL and grace?

We are not since we treat zero as the lack of TTL limit for the client. Let's revisit that specific point once unset is implemented and we treat NAN as the lack of limit instead.

[hermunn]

This is waiting list serialization once again

Yeah, you are right, my comment does not add much, and I think we should simply go with the PR as it is now, with all the timestamp confusion which exist.

[hermunn]

Sorry, strike that, I still think we need a test case demonstrating how you can get a hit, even when the request set both req.ttl and req.grace to 0s because of the waiting list and timestamps.

[AlveElde]

If you, like me, were wondering why the test case provided does not return a grace hit, this is the reason: https://github.com/varnishcache/varnish-cache/pull/2422. The object in cache is still fresh, and since we ignore req.ttl, we get no candidates for a grace hit. Whether this is the correct behavior is a subject for future discussion.

[dridi]

I think the trade off from #2422 can be let go in favor of the trade off from #4032. You should end up with cache hits again, for a different reason.

[AlveElde]

I force pushed for two reasons:

• Squash the squashme commit
• Drop the commit changing the default value of req.ttl and req.grace to NAN

CCI alerted me to the fact that a VTC was failing due to a NAN being converted to string in the following line:

set resp.http.X-req-grace = req.grace;

Unless we want to convert NAN to some number when reading req.ttl and req.grace, I suggest we stick with -1 as the default value.

[nigoroll]

Unless we want to convert NAN to some number when reading req.ttl and req.grace, I suggest we stick with -1 as the default value.

I think we should print NAN

[dridi]

This pull request didn't strictly need #4069 to move forward, but #4069 cemented the semantics for NAN in a vtim_dur. We should use that for req.ttl and req.grace now.