VBF: Move 304 logic to builtin VCL

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 {

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))

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 == <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.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 == <undef>
+	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 == <undef>
+	expect resp.http.be304-2 == <undef>
+	expect resp.http.be304-3 == true
+	expect resp.http.vbref == true
+	expect resp.http.vbresp == true
+	expect resp.http.from-bo == <undef>
+} -run

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 == <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

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 {