[master] 61ee440b5 VBF: Move 304 logic to builtin VCL
Walid Boudebouda
walid.boudebouda at gmail.com
Mon Sep 1 08:46:05 UTC 2025
commit 61ee440b5f427e0c3aa60b6b285deeaada86c99b
Author: Walid Boudebouda <walid.boudebouda at gmail.com>
Date: Mon Aug 26 14:01:37 2024 +0200
VBF: Move 304 logic to builtin VCL
diff --git a/bin/varnishd/builtin.vcl b/bin/varnishd/builtin.vcl
index a6f8c79c9..bf7595ebc 100644
--- a/bin/varnishd/builtin.vcl
+++ b/bin/varnishd/builtin.vcl
@@ -204,15 +204,44 @@ sub vcl_builtin_backend_fetch {
}
}
+sub vcl_backend_refresh {
+ call vcl_builtin_backend_refresh;
+ return (merge);
+}
+
+sub vcl_builtin_backend_refresh {
+ call vcl_refresh_valid;
+ call vcl_refresh_conditions;
+ call vcl_refresh_status;
+}
+
+# Check that the stale object was not invalidated under our feet
+sub vcl_refresh_valid {
+ if (!obj_stale.is_valid) {
+ return (error(503, "Invalid object for refresh"));
+ }
+}
+
+# Only allow revalidation if we asked for it
+sub vcl_refresh_conditions {
+ if (!bereq.http.if-modified-since &&
+ !bereq.http.if-none-match) {
+ return (error(503, "Unexpected 304"));
+ }
+}
+
+# We currently only revalidate 200 responses
+sub vcl_refresh_status {
+ if (obj_stale.status != 200) {
+ return (error(503, "Invalid object for refresh"));
+ }
+}
+
sub vcl_backend_response {
call vcl_builtin_backend_response;
return (deliver);
}
-sub vcl_backend_refresh {
- return (merge);
-}
-
sub vcl_builtin_backend_response {
call vcl_beresp_range;
if (bereq.uncacheable) {
diff --git a/bin/varnishd/cache/cache_fetch.c b/bin/varnishd/cache/cache_fetch.c
index fe957f64b..0fc041111 100644
--- a/bin/varnishd/cache/cache_fetch.c
+++ b/bin/varnishd/cache/cache_fetch.c
@@ -351,37 +351,23 @@ 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);
}
/*--------------------------------------------------------------------
@@ -392,8 +378,9 @@ static const struct fetch_step * v_matchproto_(vbf_state_f)
vbf_stp_startfetch(struct worker *wrk, struct busyobj *bo)
{
int i;
+ const char *q;
vtim_real now;
- unsigned handling;
+ unsigned handling, skip_vbr = 0;
struct objcore *oc;
CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
@@ -489,14 +476,66 @@ 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) && !bo->uncacheable) {
+ if (bo->stale_oc == NULL){
+ VSLb(bo->vsl, SLT_Error,
+ "304 response but not conditional fetch");
+ bo->htc->doclose = SC_RX_BAD;
+ vbf_cleanup(bo);
+ return (F_STP_ERROR);
+ }
+ bo->was_304 = 1;
+ VCL_backend_refresh_method(bo->vcl, wrk, NULL, bo, NULL);
+ switch (wrk->vpi->handling) {
+ case VCL_RET_MERGE:
+ vbf_304_logic(bo);
+ break;
+ case VCL_RET_BERESP:
+ http_SetStatus(bo->beresp, 200, NULL);
+ http_ForceHeader(bo->beresp, H_Content_Length,
+ HTTP_GetHdrPack(wrk, bo->stale_oc,
+ H_Content_Length));
+ http_Unset(bo->beresp, H_Content_Encoding);
+ q = HTTP_GetHdrPack(wrk, bo->stale_oc,
+ H_Content_Encoding);
+ if (q != NULL) {
+ http_ForceHeader(bo->beresp,
+ H_Content_Encoding, q);
+ }
+ q = HTTP_GetHdrPack(wrk, bo->stale_oc,
+ H_Last_Modified);
+ if (q != NULL) {
+ http_ForceHeader(bo->beresp,
+ H_Last_Modified, q);
+ }
+ q = HTTP_GetHdrPack(wrk, bo->stale_oc, H_ETag);
+ if (q != NULL)
+ http_ForceHeader(bo->beresp, H_ETag, q);
+ break;
+ case VCL_RET_OBJ_STALE:
+ if (HTTP_Decode(bo->beresp, ObjGetAttr(bo->wrk,
+ bo->stale_oc, OA_HEADERS, NULL))) {
+ bo->htc->doclose = SC_RX_OVERFLOW;
+ vbf_cleanup(bo);
+ return (F_STP_ERROR);
+ }
+ break;
+ case VCL_RET_RETRY:
+ case VCL_RET_ERROR:
+ case VCL_RET_ABANDON:
+ case VCL_RET_FAIL:
+ skip_vbr = 1;
+ break;
+ default:
+ WRONG("Illegal return from vcl_backend_refresh{}");
+ }
+ }
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/b00094.vtc b/bin/varnishtest/tests/b00094.vtc
new file mode 100644
index 000000000..92a17bcff
--- /dev/null
+++ b/bin/varnishtest/tests/b00094.vtc
@@ -0,0 +1,91 @@
+varnishtest "Test vcl_backend_refresh"
+
+server s1 {
+ rxreq
+ txresp -hdr "Etag: abcd" -hdr "from-be: true" -body "abcdefghij"
+
+ 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.ttl = 0.01s;
+ set beresp.grace = 0s;
+ set beresp.keep = 10m;
+ set beresp.http.was-304 = beresp.was_304;
+ }
+
+ sub vcl_backend_refresh {
+ if (bereq.http.stale) {
+ return (obj_stale);
+ } else if (bereq.http.beresp) {
+ return (beresp);
+ }
+ }
+
+} -start
+
+# insert object in cache
+client c1 {
+ txreq
+ rxresp
+ expect resp.status == 200
+ expect resp.http.was-304 == false
+ expect resp.http.from-be == true
+ expect resp.bodylen == 10
+ expect resp.body == "abcdefghij"
+} -run
+
+# let the object die
+delay 0.01
+
+# should get the stale obj headers only
+client c2 {
+ txreq -hdr "stale: 1"
+ rxresp
+ expect resp.status == 200
+ expect resp.http.be304-1 == <undef>
+ expect resp.http.from-be == true
+ expect resp.bodylen == 10
+ expect resp.body == "abcdefghij"
+} -run
+
+# let the object die
+delay 0.01
+
+# get the beresp headers only
+client c3 {
+ txreq -hdr "beresp: 1"
+ rxresp
+ expect resp.status == 200
+ expect resp.http.from-bo == <undef>
+ expect resp.http.be304-2 == true
+ expect resp.bodylen == 10
+ expect resp.body == "abcdefghij"
+} -run
+
+# let the object die
+delay 0.01
+
+# default, get a merge of stale_oc and beresp headers
+client c4 {
+ txreq
+ rxresp
+ expect resp.status == 200
+ expect resp.http.from-bo == <undef>
+ expect resp.http.be304-2 == true
+ expect resp.http.be304-3 == true
+ expect resp.bodylen == 10
+ expect resp.body == "abcdefghij"
+} -run
diff --git a/bin/varnishtest/tests/b00095.vtc b/bin/varnishtest/tests/b00095.vtc
new file mode 100644
index 000000000..681da0fc3
--- /dev/null
+++ b/bin/varnishtest/tests/b00095.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 == <undef>
+ 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 != <undef>
+ expect resp.http.ttl != <undef>
+ expect resp.http.uncacheable == false
+} -run
More information about the varnish-commit
mailing list