diff --git a/bin/varnishd/builtin.vcl b/bin/varnishd/builtin.vcl index 74dde825970..41f6dc51f43 100644 --- a/bin/varnishd/builtin.vcl +++ b/bin/varnishd/builtin.vcl @@ -210,7 +210,44 @@ sub vcl_backend_response { } sub vcl_backend_refresh { - return (merge); + call vcl_builtin_backend_refresh; + return (merge); +} + +sub vcl_builtin_backend_refresh { + call vcl_refresh_valid; + call vcl_refresh_conditions; + call vcl_refresh_status; +} + +sub vcl_refresh_valid { + if (obj_stale.retried) { # read-only, analogous to bereq.retries, but BOOL + return (error); + } + if (!obj_stale.is_valid) { + call vcl_refresh_retry; + } +} + +sub vcl_refresh_status { + if (obj_stale.status != 200) { + call vcl_refresh_retry; + } +} + +sub vcl_refresh_conditions { + if (!bereq.http.if-modified-since && + !bereq.http.if-none-match) { + return (error); + } +} + +sub vcl_refresh_retry { + unset bereq.http.if-modified-since; + unset bereq.http.if-none-match; + # Same transition as return (fetch) from vcl_backend_fetch, + # but turns into an error if obj_stale.retried already true. + return (retry(fetch)); } sub vcl_builtin_backend_response { diff --git a/bin/varnishd/cache/cache_fetch.c b/bin/varnishd/cache/cache_fetch.c index 76a819c2c61..3c85770bda9 100644 --- a/bin/varnishd/cache/cache_fetch.c +++ b/bin/varnishd/cache/cache_fetch.c @@ -350,37 +350,24 @@ vbf_stp_retry(struct worker *wrk, struct busyobj *bo) * 304 setup logic */ -static int +static void vbf_304_logic(struct busyobj *bo) { - if (bo->stale_oc != NULL && - ObjCheckFlag(bo->wrk, bo->stale_oc, OF_IMSCAND)) { - AZ(bo->stale_oc->flags & (OC_F_HFM|OC_F_PRIVATE)); - if (ObjCheckFlag(bo->wrk, bo->stale_oc, OF_CHGCE)) { - /* - * If a VFP changed C-E in the stored - * object, then don't overwrite C-E from - * the IMS fetch, and we must weaken any - * new ETag we get. - */ - RFC2616_Weaken_Etag(bo->beresp); - } - http_Unset(bo->beresp, H_Content_Encoding); - http_Unset(bo->beresp, H_Content_Length); - HTTP_Merge(bo->wrk, bo->stale_oc, bo->beresp); - assert(http_IsStatus(bo->beresp, 200)); - bo->was_304 = 1; - } else if (!bo->uncacheable) { + + AZ(bo->stale_oc->flags & (OC_F_HFM|OC_F_PRIVATE)); + if (ObjCheckFlag(bo->wrk, bo->stale_oc, OF_CHGCE)) { /* - * Backend sent unallowed 304 + * If a VFP changed C-E in the stored + * object, then don't overwrite C-E from + * the IMS fetch, and we must weaken any + * new ETag we get. */ - VSLb(bo->vsl, SLT_Error, - "304 response but not conditional fetch"); - bo->htc->doclose = SC_RX_BAD; - vbf_cleanup(bo); - return (-1); + RFC2616_Weaken_Etag(bo->beresp); } - return (1); + http_Unset(bo->beresp, H_Content_Encoding); + http_Unset(bo->beresp, H_Content_Length); + HTTP_Merge(bo->wrk, bo->stale_oc, bo->beresp); + assert(http_IsStatus(bo->beresp, 200)); } /*-------------------------------------------------------------------- @@ -392,7 +379,7 @@ vbf_stp_startfetch(struct worker *wrk, struct busyobj *bo) { int i; vtim_real now; - unsigned handling; + unsigned handling, retried_stale, skip_vbr = 0; struct objcore *oc; CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC); @@ -407,8 +394,10 @@ vbf_stp_startfetch(struct worker *wrk, struct busyobj *bo) http_Unset(bo->bereq, "\012X-Varnish:"); http_PrintfHeader(bo->bereq, "X-Varnish: %ju", VXID(bo->vsl->wid)); - - VCL_backend_fetch_method(bo->vcl, wrk, NULL, bo, NULL); + if (!bo->retried_stale) + VCL_backend_fetch_method(bo->vcl, wrk, NULL, bo, NULL); + else + wrk->vpi->handling = VCL_RET_FETCH; if (wrk->vpi->handling == VCL_RET_ABANDON || wrk->vpi->handling == VCL_RET_FAIL) @@ -485,14 +474,49 @@ vbf_stp_startfetch(struct worker *wrk, struct busyobj *bo) AZ(bo->do_esi); AZ(bo->was_304); - if (http_IsStatus(bo->beresp, 304) && vbf_304_logic(bo) < 0) - return (F_STP_ERROR); + if (http_IsStatus(bo->beresp, 304)) { + if (bo->stale_oc != NULL) { + retried_stale = bo->retried_stale; + VCL_backend_refresh_method(bo->vcl, wrk, NULL, bo, NULL); + bo->was_304 = 1; + switch (wrk->vpi->handling) { + case VCL_RET_MERGE: + vbf_304_logic(bo); + break; + case VCL_RET_BERESP: + break; + case VCL_RET_OBJ_STALE: + HTTP_Decode(bo->beresp, ObjGetAttr(bo->wrk, bo->stale_oc, OA_HEADERS, NULL)); + break; + case VCL_RET_RETRY: + if (retried_stale == 1) { + VSLb(bo->vsl, SLT_VCL_Error, + "Conditional fetch already retried, delivering 503"); + return (F_STP_ERROR); + } + /* FALLTHROUGH */ + case VCL_RET_ERROR: + case VCL_RET_ABANDON: + case VCL_RET_FAIL: + skip_vbr = 1; + break; + default: + WRONG("Illegal return from vcl_backend_refresh{}"); + } + } else if (!bo->uncacheable){ + VSLb(bo->vsl, SLT_Error, + "304 response but not conditional fetch"); + bo->htc->doclose = SC_RX_BAD; + vbf_cleanup(bo); + return (F_STP_ERROR); + } + } if (bo->htc != NULL && bo->htc->doclose == SC_NULL && http_GetHdrField(bo->bereq, H_Connection, "close", NULL)) bo->htc->doclose = SC_REQ_CLOSE; - - VCL_backend_response_method(bo->vcl, wrk, NULL, bo, NULL); + if (!skip_vbr) + VCL_backend_response_method(bo->vcl, wrk, NULL, bo, NULL); if (bo->htc != NULL && bo->htc->doclose == SC_NULL && http_GetHdrField(bo->beresp, H_Connection, "close", NULL)) diff --git a/bin/varnishtest/tests/b00089.vtc b/bin/varnishtest/tests/b00089.vtc new file mode 100644 index 00000000000..015e653b1c3 --- /dev/null +++ b/bin/varnishtest/tests/b00089.vtc @@ -0,0 +1,123 @@ +varnishtest "Test vcl_backend_refresh" + +server s1 { + rxreq + txresp -hdr "Etag: abcd" -hdr "from-bo: true" -bodylen 10 + + rxreq + expect req.http.if-none-match == "abcd" + txresp -status 304 -hdr "be304-1: true" + + rxreq + expect req.http.if-none-match == "abcd" + txresp -status 304 -hdr "be304-2: true" + + rxreq + expect req.http.if-none-match == "abcd" + txresp -status 304 -hdr "be304-3: true" +} -start + +varnish v1 -vcl+backend { + + sub vcl_backend_response { + set beresp.http.vbresp = "true"; + set beresp.ttl = 0.01s; + set beresp.grace = 0s; + set beresp.keep = 10m; + set beresp.http.was-304 = beresp.was_304; + } + + sub vcl_backend_refresh { + set beresp.http.vbref = "true"; + } + +} -start + +client c1 { + txreq + rxresp + expect resp.status == 200 + expect resp.http.was-304 == false + expect resp.http.vbref == + expect resp.http.vbresp == true + expect resp.http.from-bo == true +} -run + +delay 0.01 + +client c2 { + txreq + rxresp + expect resp.status == 200 + expect resp.http.was-304 == true + expect resp.http.be304-1 == true + expect resp.http.vbref == true + expect resp.http.vbresp == true + expect resp.http.from-bo == true +} -run + +varnish v1 -vcl+backend { + + sub vcl_backend_response { + set beresp.http.vbresp = "true"; + set beresp.ttl = 0.01s; + set beresp.grace = 0s; + set beresp.keep = 10m; + set beresp.http.was-304 = beresp.was_304; + } + + + sub vcl_refresh_status { + set beresp.http.vbref = "true"; + if (obj_stale.status == 200) { + return (obj_stale); + } + } +} + +delay 0.01 + +client c3 { + txreq + rxresp + expect resp.status == 200 + expect resp.http.was-304 == true + expect resp.http.be304-1 == true + expect resp.http.be304-2 == + expect resp.http.vbref == true + expect resp.http.vbresp == true + expect resp.http.from-bo == true +} -run + +varnish v1 -vcl+backend { + + sub vcl_backend_response { + set beresp.http.vbresp = "true"; + set beresp.ttl = 0.01s; + set beresp.grace = 0s; + set beresp.keep = 10m; + set beresp.http.was-304 = beresp.was_304; + } + + sub vcl_refresh_status { + set beresp.http.vbref = "true"; + if (obj_stale.status == 200) { + return (beresp); + } + } +} + +delay 0.01 + +client c4 { + txreq + rxresp + expect resp.status == 304 + expect resp.http.was-304 == true + expect resp.http.be304-1 == + expect resp.http.be304-2 == + expect resp.http.be304-3 == true + expect resp.http.vbref == true + expect resp.http.vbresp == true + expect resp.http.from-bo == +} -run diff --git a/bin/varnishtest/tests/b00090.vtc b/bin/varnishtest/tests/b00090.vtc new file mode 100644 index 00000000000..681da0fc32f --- /dev/null +++ b/bin/varnishtest/tests/b00090.vtc @@ -0,0 +1,78 @@ +varnishtest "Test obj_stale vcl variables" + +server s1 { + rxreq + txresp -hdr "Etag: abcd" -hdr "from-bo: true" -bodylen 10 + rxreq + expect req.http.if-none-match == "abcd" + txresp -status 304 +} -start + +varnish v1 -vcl+backend { + + sub vcl_backend_response { + set beresp.http.vbresp = "true"; + set beresp.ttl = 0.01s; + set beresp.grace = 0s; + set beresp.keep = 10m; + set beresp.http.was-304 = beresp.was_304; + } + + sub vcl_refresh_status { + if (obj_stale.status == 200) { + set beresp.http.vbref = "true"; + + set beresp.http.http = obj_stale.http.from-bo; + set beresp.http.age = obj_stale.age; + set beresp.http.can_esi = obj_stale.can_esi; + set beresp.http.grace = obj_stale.grace; + set beresp.http.hits = obj_stale.hits; + set beresp.http.keep = obj_stale.keep; + set beresp.http.proto = obj_stale.proto; + set beresp.http.reason = obj_stale.reason; + set beresp.http.status = obj_stale.status; + set beresp.http.storage = obj_stale.storage; + set beresp.http.time = obj_stale.time; + set beresp.http.ttl = obj_stale.ttl; + set beresp.http.uncacheable = obj_stale.uncacheable; + } + return (merge); + } +} -start + +client c1 { + txreq + rxresp + expect resp.status == 200 + + expect resp.http.was-304 == false + expect resp.http.vbref == + expect resp.http.vbresp == true + expect resp.http.from-bo == true +} -run + +delay 0.01 + +client c2 { + txreq + rxresp + expect resp.status == 200 + expect resp.http.was-304 == true + expect resp.http.vbresp == true + expect resp.http.vbref == true + expect resp.http.from-bo == true + + expect resp.http.http == true + expect resp.http.age == 0 + expect resp.http.can_esi == false + expect resp.http.grace == 0.000 + expect resp.http.hits == 0 + expect resp.http.keep == 600.000 + expect resp.http.proto == HTTP/1.1 + expect resp.http.reason == OK + expect resp.http.status == 200 + expect resp.http.storage == storage.s0 + expect resp.http.time != + expect resp.http.ttl != + expect resp.http.uncacheable == false +} -run diff --git a/bin/varnishtest/tests/r01672.vtc b/bin/varnishtest/tests/r01672.vtc index e59670d7943..077de1a3ca4 100644 --- a/bin/varnishtest/tests/r01672.vtc +++ b/bin/varnishtest/tests/r01672.vtc @@ -17,6 +17,12 @@ varnish v1 -vcl+backend { set beresp.grace = 0s; set beresp.keep = 10s; } + + sub vcl_refresh_status { + if (obj_stale.status != 200) { + return (error); + } + } } -start client c1 {