[6.0] f80c03d68 Reintroduce the req.grace variable, change keep behavior
Dridi Boukelmoune
dridi.boukelmoune at gmail.com
Thu Aug 16 08:53:17 UTC 2018
commit f80c03d682720be5d4e59d5ca53c829028671270
Author: Pål Hermunn Johansen <hermunn at varnish-software.com>
Date: Tue Jun 12 16:33:07 2018 +0200
Reintroduce the req.grace variable, change keep behavior
The req.grace variable can be set in vcl_recv to cap the grace
of objects in the cache, in the same way as in 3.0.x
The "keep" behavior changes with this patch. We now always go
to vcl_miss when the expired object is out of grace, or we go
to the waiting list. The result is that it is no longer
possible to deliver a "keep" object in vcl_hit.
Note that when we get to vcl_miss, we will still have the 304
candidate, but without the detour by vcl_hit.
This commit changes VCL, but only slightly, so we aim to back
port this to earlier versions of Varnish Cache.
Refs: #1799 and #2519
diff --git a/bin/varnishd/cache/cache.h b/bin/varnishd/cache/cache.h
index 2298feaad..adf9e34a9 100644
--- a/bin/varnishd/cache/cache.h
+++ b/bin/varnishd/cache/cache.h
@@ -470,6 +470,7 @@ struct req {
uint8_t digest[DIGEST_LEN];
double d_ttl;
+ double d_grace;
ssize_t req_bodybytes; /* Parsed req bodybytes */
const struct stevedore *storage;
diff --git a/bin/varnishd/cache/cache_expire.c b/bin/varnishd/cache/cache_expire.c
index efe3814c6..6aa1ae07c 100644
--- a/bin/varnishd/cache/cache_expire.c
+++ b/bin/varnishd/cache/cache_expire.c
@@ -74,6 +74,24 @@ EXP_Ttl(const struct req *req, const struct objcore *oc)
return (oc->t_origin + r);
}
+/*--------------------------------------------------------------------
+ * Calculate an object's effective ttl+grace time, taking req.grace into
+ * account if it is available.
+ */
+
+double
+EXP_Ttl_grace(const struct req *req, const struct objcore *oc)
+{
+ double g;
+
+ CHECK_OBJ_NOTNULL(oc, OBJCORE_MAGIC);
+
+ g = oc->grace;
+ if (req != NULL && req->d_grace >= 0. && req->d_grace < g)
+ g = req->d_grace;
+ return (EXP_Ttl(req, oc) + g);
+}
+
/*--------------------------------------------------------------------
* Post an objcore to the exp_thread's inbox.
*/
diff --git a/bin/varnishd/cache/cache_hash.c b/bin/varnishd/cache/cache_hash.c
index 55a451cf7..38531b149 100644
--- a/bin/varnishd/cache/cache_hash.c
+++ b/bin/varnishd/cache/cache_hash.c
@@ -478,36 +478,50 @@ HSH_Lookup(struct req *req, struct objcore **ocp, struct objcore **bocp,
busy_found = 0;
}
+ if (exp_oc != NULL && EXP_Ttl_grace(req, exp_oc) < req->t_req) {
+ /* the newest object is out of grace */
+ if (!busy_found) {
+ /*
+ * here we get to insert the busy object. This
+ * translates to a MISS with a 304 candidate.
+ */
+ assert(oh->refcnt > 1);
+ assert(exp_oc->objhead == oh);
+ exp_oc->refcnt++;
+ /* Insert objcore in objecthead and release mutex */
+ *bocp = hsh_insert_busyobj(wrk, oh);
+ /* NB: no deref of objhead, new object inherits reference */
+ Lck_Unlock(&oh->mtx);
+ *ocp = exp_oc;
+ return (HSH_MISS);
+ } else {
+ /* we have no use for this very expired object */
+ exp_oc = NULL;
+ }
+ }
if (exp_oc != NULL) {
+ /*
+ * here the object is within grace, so we expect it to
+ * be delivered
+ */
assert(oh->refcnt > 1);
assert(exp_oc->objhead == oh);
+ exp_oc->refcnt++;
if (!busy_found) {
*bocp = hsh_insert_busyobj(wrk, oh);
retval = HSH_EXPBUSY;
} else {
AZ(req->hash_ignore_busy);
- /*
- * here we have a busy object, but if the stale object
- * is not under grace we go to the waiting list
- * instead of returning the stale object
- */
- if (EXP_Ttl(req, exp_oc) + exp_oc->grace < req->t_req)
- retval = HSH_BUSY;
- else
- retval = HSH_EXP;
- }
- if (retval != HSH_BUSY) {
- exp_oc->refcnt++;
-
- if (exp_oc->hits < LONG_MAX)
- exp_oc->hits++;
- Lck_Unlock(&oh->mtx);
- if (retval == HSH_EXP)
- assert(HSH_DerefObjHead(wrk, &oh));
- *ocp = exp_oc;
- return (retval);
+ retval = HSH_EXP;
}
+ if (exp_oc->hits < LONG_MAX)
+ exp_oc->hits++;
+ Lck_Unlock(&oh->mtx);
+ if (retval == HSH_EXP)
+ assert(HSH_DerefObjHead(wrk, &oh));
+ *ocp = exp_oc;
+ return (retval);
}
if (!busy_found) {
diff --git a/bin/varnishd/cache/cache_req_fsm.c b/bin/varnishd/cache/cache_req_fsm.c
index b55a0c007..83ddc4760 100644
--- a/bin/varnishd/cache/cache_req_fsm.c
+++ b/bin/varnishd/cache/cache_req_fsm.c
@@ -508,14 +508,23 @@ cnt_lookup(struct worker *wrk, struct req *req)
AZ(req->objcore);
if (lr == HSH_MISS) {
- /* Found nothing */
- AZ(oc);
- if (busy != NULL) {
+ /* Found nothing or an object that was out of grace */
+ if (oc == NULL) {
+ /* Nothing */
+ if (busy != NULL) {
+ AN(busy->flags & OC_F_BUSY);
+ req->objcore = busy;
+ req->req_step = R_STP_MISS;
+ } else {
+ req->req_step = R_STP_PASS;
+ }
+ } else {
+ /* Object out of grace */
+ AN(busy); /* Else we should be on waiting list */
AN(busy->flags & OC_F_BUSY);
req->objcore = busy;
+ req->stale_oc = oc;
req->req_step = R_STP_MISS;
- } else {
- req->req_step = R_STP_PASS;
}
return (REQ_FSM_MORE);
}
@@ -805,6 +814,7 @@ cnt_recv_prep(struct req *req, const char *ci)
AN(req->director_hint);
req->d_ttl = -1;
+ req->d_grace = -1;
req->disable_esi = 0;
req->hash_always_miss = 0;
req->hash_ignore_busy = 0;
diff --git a/bin/varnishd/cache/cache_varnishd.h b/bin/varnishd/cache/cache_varnishd.h
index af122f768..e967f1d7f 100644
--- a/bin/varnishd/cache/cache_varnishd.h
+++ b/bin/varnishd/cache/cache_varnishd.h
@@ -127,6 +127,7 @@ void VDI_Init(void);
/* cache_exp.c */
double EXP_Ttl(const struct req *, const struct objcore *);
+double EXP_Ttl_grace(const struct req *, const struct objcore *oc);
void EXP_Insert(struct worker *wrk, struct objcore *oc);
void EXP_Remove(struct objcore *);
diff --git a/bin/varnishd/cache/cache_vrt_var.c b/bin/varnishd/cache/cache_vrt_var.c
index c06dde561..c902bee4c 100644
--- a/bin/varnishd/cache/cache_vrt_var.c
+++ b/bin/varnishd/cache/cache_vrt_var.c
@@ -435,7 +435,7 @@ VRT_r_obj_storage(VRT_CTX)
/*--------------------------------------------------------------------*/
-#define REQ_VAR_L(nm, elem, type,extra) \
+#define REQ_VAR_L(nm, elem, type, extra) \
\
VCL_VOID \
VRT_l_req_##nm(VRT_CTX, type arg) \
@@ -460,6 +460,8 @@ REQ_VAR_L(backend_hint, director_hint, VCL_BACKEND,)
REQ_VAR_R(backend_hint, director_hint, VCL_BACKEND)
REQ_VAR_L(ttl, d_ttl, VCL_DURATION, if (!(arg>0.0)) arg = 0;)
REQ_VAR_R(ttl, d_ttl, VCL_DURATION)
+REQ_VAR_L(grace, d_grace, VCL_DURATION, if (!(arg>0.0)) arg = 0;)
+REQ_VAR_R(grace, d_grace, VCL_DURATION)
/*--------------------------------------------------------------------*/
diff --git a/bin/varnishtest/tests/b00098.vtc b/bin/varnishtest/tests/b00062.vtc
similarity index 100%
rename from bin/varnishtest/tests/b00098.vtc
rename to bin/varnishtest/tests/b00062.vtc
diff --git a/bin/varnishtest/tests/b00064.vtc b/bin/varnishtest/tests/b00064.vtc
new file mode 100644
index 000000000..6054ec1e6
--- /dev/null
+++ b/bin/varnishtest/tests/b00064.vtc
@@ -0,0 +1,126 @@
+varnishtest "Test that req.grace will hold a client when a miss is anticipated"
+
+barrier b1 cond 2
+
+server s1 {
+ rxreq
+ expect req.url == "/"
+ txresp -body "0"
+
+ rxreq
+ expect req.url == "/"
+ txresp -body "1"
+
+ # second time we get a request, we use some time to serve it
+ rxreq
+ expect req.url == "/"
+ barrier b1 sync
+ delay .1
+ txresp -body "2"
+
+ # Last request, to a different URL to catch it if varnish asks for "/" too many times
+ rxreq
+ expect req.url == "/2"
+ txresp -body "x"
+} -start
+
+varnish v1 -vcl+backend {
+ import std;
+
+ sub vcl_recv {
+ # When we know in vcl_recv that we will have no grace, it is now
+ # possible to signal this to the lookup function:
+ if (req.http.X-no-grace) {
+ set req.grace = 0s;
+ }
+ }
+ sub vcl_hit {
+ set req.http.X-grace = obj.grace;
+ }
+ sub vcl_backend_response {
+ set beresp.ttl = 0.1s;
+ set beresp.grace = 1m;
+ }
+ sub vcl_deliver {
+ if (req.http.X-grace) {
+ set resp.http.X-grace = req.http.X-grace;
+ set resp.http.X-req-grace = req.grace;
+ }
+ }
+} -start
+
+client c1 {
+ txreq
+ rxresp
+ expect resp.status == 200
+ expect resp.body == "0"
+ expect resp.http.X-grace == <undef>
+ # let the object's ttl expire
+ delay .2
+ # get a genuinely fresh object by disabling grace
+ # we will not get to vcl_hit to see the grace
+ txreq -hdr "X-no-grace: true"
+ rxresp
+ expect resp.status == 200
+ expect resp.body == "1"
+ expect resp.http.X-grace == <undef>
+ expect resp.http.X-req-grace == <undef>
+} -run
+
+# let the latest object's ttl expire.
+delay .2
+
+varnish v1 -expect n_object == 1
+
+# c2 asks for the object under grace
+client c2 {
+ txreq
+ rxresp
+ # we did not disable grace in the request, so we should get the graced object here
+ expect resp.status == 200
+ expect resp.body == "1"
+ expect resp.http.X-grace == "60.000"
+ expect resp.http.X-req-grace < 0.
+} -start
+
+delay .1
+
+# c3 asks for graced object, but now we disable grace.
+client c3 {
+ txreq -hdr "X-no-grace: true"
+ rxresp
+ expect resp.status == 200
+ # Here we have disable grace and should get the object from the background fetch,
+ # which will take us into vcl_hit
+ expect resp.body == "2"
+ expect resp.http.X-grace == "60.000"
+ expect resp.http.X-req-grace == "0.000"
+} -start
+
+delay .1
+
+# c4 does not disable grace, and should get the grace object even
+# though c3 is waiting on the background thread to deliver a new
+# version.
+
+client c4 {
+ txreq
+ rxresp
+ barrier b1 sync
+ expect resp.status == 200
+ # We should get what c1 got in the very beginning
+ expect resp.body == "1"
+ expect resp.http.X-grace == "60.000"
+ expect resp.http.X-req-grace < 0.
+} -start
+
+client c2 -wait
+client c3 -wait
+client c4 -wait
+
+client c5 {
+ txreq -url "/2"
+ rxresp
+ expect resp.status == 200
+ expect resp.body == "x"
+} -run
diff --git a/bin/varnishtest/tests/r02705.vtc b/bin/varnishtest/tests/r02705.vtc
new file mode 100644
index 000000000..54bec95c2
--- /dev/null
+++ b/bin/varnishtest/tests/r02705.vtc
@@ -0,0 +1,51 @@
+varnishtest "No vcl_hit when grace has run out, with working IMS"
+
+server s1 {
+ rxreq
+ txresp -hdr {Etag: "Foo"} -body "1"
+
+ rxreq
+ txresp -status 304
+
+ rxreq
+ expect req.url == "/2"
+ txresp -body "3"
+} -start
+
+varnish v1 -vcl+backend {
+ sub vcl_backend_response {
+ set beresp.ttl = 1ms;
+ set beresp.grace = 0s;
+ set beresp.keep = 1m;
+ if (bereq.http.Hit) {
+ set beresp.http.Hit = bereq.http.Hit;
+ }
+ }
+ sub vcl_hit {
+ set req.http.Hit = "HIT";
+ }
+} -start
+
+client c1 {
+ txreq
+ rxresp
+ expect resp.http.Hit == <undef>
+ expect resp.http.Etag == {"Foo"}
+ expect resp.status == 200
+ expect resp.body == "1"
+ delay .2
+ txreq
+ rxresp
+ expect resp.http.Hit == <undef>
+ expect resp.http.Etag == {"Foo"}
+ expect resp.status == 200
+ expect resp.body == "1"
+} -run
+
+client c2 {
+ txreq -url "/2"
+ rxresp
+ expect resp.http.Hit == <undef>
+ expect resp.body == "3"
+} -run
+
diff --git a/doc/sphinx/reference/vcl_var.rst b/doc/sphinx/reference/vcl_var.rst
index 30fee32d7..b08b947fb 100644
--- a/doc/sphinx/reference/vcl_var.rst
+++ b/doc/sphinx/reference/vcl_var.rst
@@ -304,6 +304,21 @@ req.ttl
Deprecated and scheduled for removal with varnish release 7.
+req.grace
+
+ Type: DURATION
+
+ Readable from: client
+
+ Writable from: client
+
+
+ Upper limit on the object grace.
+
+ During lookup the minimum of req.grace and the object's stored
+ grace value will be used as the object's grace.
+
+
req.xid
Type: STRING
More information about the varnish-commit
mailing list