-
Notifications
You must be signed in to change notification settings - Fork 381
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
expire: Force a cache MISS when req.ttl = 0s #4041
Open
AlveElde
wants to merge
3
commits into
varnishcache:master
Choose a base branch
from
AlveElde:req-ttl-0s
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,100 @@ | ||
varnishtest "req.ttl = 0s forces cache MISS with request coalescing" | ||
|
||
barrier b1 cond 3 -cyclic | ||
barrier b2 sock 2 -cyclic | ||
|
||
server s1 { | ||
rxreq | ||
txresp -nolen -hdr "Content-Length: 3" -hdr "version: 1" | ||
barrier b1 sync | ||
send "foo" | ||
|
||
rxreq | ||
txresp -nolen -hdr "Content-Length: 3" -hdr "version: 2" | ||
barrier b1 sync | ||
send "bar" | ||
} -start | ||
|
||
varnish v1 -vcl+backend { | ||
import std; | ||
import vtc; | ||
|
||
sub vcl_recv { | ||
if (req.http.ttl) { | ||
set req.ttl = std.duration(req.http.ttl, 0s); | ||
} | ||
|
||
// Make sure both client tasks are started before the fetch is | ||
// initiated to guarantee that one ends up on the waitinglist. | ||
if (req.http.sync) { | ||
vtc.barrier_sync("${b2_sock}"); | ||
} | ||
} | ||
} -start | ||
|
||
# c1 and c2 send a request and sync on the barrier only after having received | ||
# headers. This ensures that the two requests are coalesced. | ||
|
||
client c1 { | ||
txreq -hdr "sync: yes" -hdr "ttl: 0s" | ||
rxresphdrs | ||
expect resp.status == 200 | ||
expect resp.http.version == "1" | ||
barrier b1 sync | ||
rxrespbody | ||
expect resp.body == "foo" | ||
} -start | ||
|
||
client c2 { | ||
txreq -hdr "sync: yes" -hdr "ttl: 0s" | ||
rxresphdrs | ||
expect resp.status == 200 | ||
expect resp.http.version == "1" | ||
barrier b1 sync | ||
rxrespbody | ||
expect resp.body == "foo" | ||
} -start | ||
|
||
client c1 -wait | ||
client c2 -wait | ||
|
||
# c3 checks that a positive or unset req.ttl gets the cached object. | ||
|
||
client c3 { | ||
txreq -hdr "ttl: 10s" | ||
rxresp | ||
expect resp.status == 200 | ||
expect resp.http.version == "1" | ||
expect resp.body == "foo" | ||
|
||
txreq | ||
rxresp | ||
expect resp.status == 200 | ||
expect resp.http.version == "1" | ||
expect resp.body == "foo" | ||
} -run | ||
|
||
# c4 and c5 force a cache MISS, where the two requests are again coalesced. | ||
|
||
client c4 { | ||
txreq -hdr "sync: yes" -hdr "ttl: 0s" | ||
rxresphdrs | ||
expect resp.status == 200 | ||
expect resp.http.version == "2" | ||
barrier b1 sync | ||
rxrespbody | ||
expect resp.body == "bar" | ||
} -start | ||
|
||
client c5 { | ||
txreq -hdr "sync: yes" -hdr "ttl: 0s" | ||
rxresphdrs | ||
expect resp.status == 200 | ||
expect resp.http.version == "2" | ||
barrier b1 sync | ||
rxrespbody | ||
expect resp.body == "bar" | ||
} -start | ||
|
||
client c4 -wait | ||
client c5 -wait |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
varnishtest "unset req.ttl and req.grace" | ||
|
||
server s1 { | ||
rxreq | ||
expect req.http.ttl-initial == "NAN" | ||
expect req.http.grace-initial == "NAN" | ||
expect req.http.ttl-set == 0.000 | ||
expect req.http.grace-set == 0.000 | ||
expect req.http.ttl-unset == "NAN" | ||
expect req.http.grace-unset == "NAN" | ||
txresp | ||
} -start | ||
|
||
varnish v1 -vcl+backend { | ||
sub vcl_recv { | ||
set req.http.ttl-initial = req.ttl; | ||
set req.http.grace-initial = req.grace; | ||
|
||
set req.ttl = 0s; | ||
set req.grace = 0s; | ||
|
||
set req.http.ttl-set = req.ttl; | ||
set req.http.grace-set = req.grace; | ||
|
||
unset req.ttl; | ||
unset req.grace; | ||
|
||
set req.http.ttl-unset = req.ttl; | ||
set req.http.grace-unset = req.grace; | ||
} | ||
} -start | ||
|
||
client c1 -repeat 2 { | ||
txreq | ||
rxresp | ||
expect resp.status == 200 | ||
} -run |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we still want to set negative arguments to zero? Could it be a better option to fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it's reasonable to expect that
req.ttl
can be set to a negative duration, we even do so in a test case:varnish-cache/bin/varnishtest/tests/v00020.vtc
Line 150 in e0b164f
Failing on a negative duration has a good chance of breaking existing VCL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a coverage test.
What are the semantics of a negative duration for
req.ttl
andreq.grace
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same as
beresp.ttl
andberesp.grace
:varnish-cache/bin/varnishd/cache/cache_vrt_var.c
Lines 743 to 744 in e0b164f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that this "has always been like that", but also please remember that for quite some time, varnish had no way of properly failing at runtime (
VRT_fail()
).The particular lines you are referring to are from 10 years ago and I am still wondering what a negative ttl/grace is supposed to mean. ttl: already expired before it even went into the cache? If those were the semantics, then we would not be allowed to take the respective objects as "just refreshed". grace: I would think a negative value should move an object into grace mode earlier than ttl.
So, as we come across these questions, we should answer them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally agree that "has always been like that" is a weak argument, but I personally prefer to limit breakage if I can. It's not obvious to me what a negative TTL/grace should mean, and I think that question is bigger than the scope of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's try to cover this quickly during the next bugwash. I am also fine with continuing to ignore negative values, but I'd like to at least ask for opinions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the point here is to cap the TTL, and once we run out of TTL the object goes in grace mode if applicable, so once we run out of TTL
req.grace
applies.This is why in my opinion
req.ttl
should only limit the ability to look up a fresh object, and that negative values that could be the result of some arithmetic expression should be bumped to zero.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dridi maybe I do not understand your answer, but why should we not fail immediately if an attempt is made to set any of the timers to a negative value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you use an expression instead of a constant to compute the acceptable TTL limit and the result is negative, I think it would be too harsh to fail the transaction.
If your "clever"
req.ttl
formula can yield something negative, then negative should be considered as allowing no TTL, just like zero.