Conditional fetch wait for streaming stale object

Wait for the stale object to become fully fetched, so that we can catch
fetch errors, before we unbusy the new object. This serves two
purposes. First it helps with request coalescing, and stops long chains of
IMS-updated short-TTL objects all streaming from a single slow body
fetch. Second it makes sure that all the object attributes are complete
when we copy them (this would be an issue for ie OA_GZIPBITS).

This patch OBE's r01646.vtc, and slightly patches r01648.vtc to expect a
503 instead of a 200 and a broken connection on the failing client.

Fixes: #3089

bin/varnishd/cache/cache_fetch.c

@@ -683,9 +683,37 @@ vbf_objiterator(void *priv, unsigned flush, const void *ptr, ssize_t len)
 static enum fetch_step
 vbf_stp_condfetch(struct worker *wrk, struct busyobj *bo)
 {
+	struct boc *stale_boc;
+	enum boc_state_e stale_state;
 
 	CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
 	CHECK_OBJ_NOTNULL(bo, BUSYOBJ_MAGIC);
+	CHECK_OBJ_NOTNULL(bo->fetch_objcore, OBJCORE_MAGIC);
+	CHECK_OBJ_NOTNULL(bo->stale_oc, OBJCORE_MAGIC);
+
+	stale_boc = HSH_RefBoc(bo->stale_oc);
+	CHECK_OBJ_ORNULL(stale_boc, BOC_MAGIC);
+	if (stale_boc) {
+		/* Wait for the stale object to become fully fetched, so
+		 * that we can catch fetch errors, before we unbusy the
+		 * new object. This serves two purposes. First it helps
+		 * with request coalesching, and stops long chains of
+		 * IMS-updated short-TTL objects all streaming from a
+		 * single slow body fetch. Second it makes sure that all
+		 * the object attributes are complete when we copy them
+		 * (this would be an issue for ie OA_GZIPBITS). */
+		ObjWaitState(bo->stale_oc, BOS_FINISHED);
+		stale_state = stale_boc->state;
+		HSH_DerefBoc(bo->wrk, bo->stale_oc);
+		stale_boc = NULL;
+		if (stale_state != BOS_FINISHED) {
+			(void)VFP_Error(bo->vfc, "Template object failed");
+			vbf_cleanup(bo);
+			wrk->stats->fetch_failed++;
+			return (F_STP_FAIL);
+		}
+	}
+	AZ(bo->stale_oc->flags & OC_F_FAILED);
 
 	AZ(vbf_beresp2obj(bo));
 
@@ -707,8 +735,6 @@ vbf_stp_condfetch(struct worker *wrk, struct busyobj *bo)
 	if (ObjIterate(wrk, bo->stale_oc, bo, vbf_objiterator, 0))
 		(void)VFP_Error(bo->vfc, "Template object failed");
 
-	if (bo->stale_oc->flags & OC_F_FAILED)
-		(void)VFP_Error(bo->vfc, "Template object failed");
 	if (bo->vfc->failed) {
 		vbf_cleanup(bo);
 		wrk->stats->fetch_failed++;

bin/varnishtest/tests/r01646.vtc

-varnishtest "cond/stream race ?"
-
-barrier b1 cond 3
-barrier b2 cond 2
-
-server s1 {
-	rxreq
-	txresp -nolen -hdr "Transfer-Encoding: chunked" -hdr "Etag: foo"
-	chunkedlen 32
-	barrier b1 sync
-	chunkedlen 32
-	barrier b2 sync
-	chunkedlen 32
-	chunkedlen 0
-} -start
-
-server s2 {
-	rxreq
-	expect req.http.if-none-match == "foo"
-	txresp -status 304
-} -start
-
-varnish v1 -vcl+backend {
-	sub vcl_backend_fetch {
-		if (bereq.http.foo == "s2") {
-			set bereq.backend = s2;
-		}
-	}
-	sub vcl_backend_response {
-		set beresp.ttl = 1s;
-		set beresp.grace = 0s;
-		set beresp.keep = 30s;
-	}
-} -start
-
-varnish v1 -cliok "param.set debug +syncvsl"
-
-client c1 {
-	txreq
-	rxresphdrs
-	barrier b1 sync
-	rxrespbody
-	expect resp.bodylen == 96
-} -start
-
-client c2 {
-	barrier b1 sync
-	delay 2
-	txreq -hdr "foo: s2"
-	rxresphdrs
-	barrier b2 sync
-	rxrespbody
-	expect resp.bodylen == 96
-} -start
-
-client c1 -wait
-client c2 -wait

bin/varnishtest/tests/r01648.vtc

@@ -62,8 +62,7 @@ client c2 {
 	txreq -hdr "Client: c2"
 	barrier b1 sync
 	rxresphdrs
-	expect resp.status == 200
-	expect resp.http.transfer-encoding == "chunked"
+	expect resp.status == 503
 } -run
 
 delay 1
@@ -76,4 +75,5 @@ client c3 {
 	expect resp.body == "abcdef"
 } -run
 
+client c1 -wait
 varnish v1 -expect fetch_failed == 2

bin/varnishtest/tests/r03089.vtc

+varnishtest "r03089 - Assert error corrupt OA_GZIPBITS"
+
+barrier b1 cond 2
+
+server s1 {
+	rxreq
+	expect req.url == /included
+	expect req.http.accept-encoding == "gzip"
+	txresp -nolen -hdr "Content-Length: 49" -hdr "Content-Encoding: gzip" -hdr {Etag: "asdf"}
+	barrier b1 sync
+	delay 1
+	sendhex "1f 8b 08 08 ef 00 22 59  02 03 5f 00 0b 2e cd 53"
+	sendhex "f0 4d ac 54 30 32 04 22  2b 03 13 2b 13 73 85 d0"
+	sendhex "10 67 05 23 03 43 73 2e  00 cf 9b db c0 1d 00 00"
+	sendhex "00"
+} -start
+
+server s2 {
+	rxreq
+	expect req.url == /included
+	expect req.http.If-None-Match == {"asdf"}
+	barrier b1 sync
+	txresp -status 304 -nolen -hdr "Etag: asdf"
+} -start
+
+server s3 {
+	rxreq
+	expect req.url == /
+	txresp -gzipbody {<esi:include src="/included"/>}
+} -start
+
+varnish v1 -arg "-p debug=+syncvsl" -vcl+backend {
+	sub vcl_deliver {
+	}
+	sub vcl_backend_fetch {
+		if (bereq.url == "/") {
+			set bereq.backend = s3;
+		} else if (bereq.http.backend == "s1") {
+			set bereq.backend = s1;
+		} else {
+			set bereq.backend = s2;
+		}
+	}
+	sub vcl_backend_response {
+		if (bereq.url == "/") {
+			set beresp.do_esi = true;
+		} else {
+			set beresp.ttl = 0.1s;
+			set beresp.grace = 0s;
+			set beresp.keep = 1m;
+		}
+	}
+} -start
+
+client c1 {
+	txreq -url / -hdr "backend: s1" -hdr "Accept-encoding: gzip"
+	rxresp
+} -start
+
+delay 1
+
+client c2 {
+	txreq -url / -hdr "backend: s2" -hdr "Accept-encoding: gzip"
+	rxresp
+} -start
+
+client c1 -wait
+client c2 -wait
+