[master] 9c10c5aca kill stale oc for backend synth and clarify when we cache errors

[Nils Goroll]

commit 9c10c5aca8b2595997f5ce490991d734d6af6928
Author: Nils Goroll <nils.goroll at uplex.de>
Date:   Sun Mar 17 11:45:18 2019 +0100

    kill stale oc for backend synth and clarify when we cache errors
    
    Fixes #2946 in the sense that we want to treat a cacheable backend synth
    like any other object and kill the stale object it replaces.
    
    We do not replace the stale oc when
    - abandoning the bereq
    - leaving vcl_backend_error with return (deliver) and beresp.ttl == 0s
    - there is a waitinglist on this object because in this case the default
      ttl would be 1s, so we might be looking at the same case as the
      previous
    
    Thank you @mbgrydeland for the review and spotting a mistake.

diff --git a/bin/varnishd/cache/cache_fetch.c b/bin/varnishd/cache/cache_fetch.c
index 33a751ea2..a7ecafa22 100644
--- a/bin/varnishd/cache/cache_fetch.c
+++ b/bin/varnishd/cache/cache_fetch.c
@@ -683,6 +683,7 @@ vbf_stp_error(struct worker *wrk, struct busyobj *bo)
 	vtim_real now;
 	uint8_t *ptr;
 	struct vsb *synth_body;
+	struct objcore *stale = NULL;
 
 	CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
 	CHECK_OBJ_NOTNULL(bo, BUSYOBJ_MAGIC);
@@ -708,6 +709,7 @@ vbf_stp_error(struct worker *wrk, struct busyobj *bo)
 	http_TimeHeader(bo->beresp, "Date: ", now);
 	http_SetHeader(bo->beresp, "Server: Varnish");
 
+	stale = bo->stale_oc;
 	bo->fetch_objcore->t_origin = now;
 	if (!VTAILQ_EMPTY(&bo->fetch_objcore->objhead->waitinglist)) {
 		/*
@@ -720,6 +722,7 @@ vbf_stp_error(struct worker *wrk, struct busyobj *bo)
 		bo->fetch_objcore->ttl = 1;
 		bo->fetch_objcore->grace = 5;
 		bo->fetch_objcore->keep = 5;
+		stale = NULL;
 	} else {
 		bo->fetch_objcore->ttl = 0;
 		bo->fetch_objcore->grace = 0;
@@ -775,6 +778,8 @@ vbf_stp_error(struct worker *wrk, struct busyobj *bo)
 	AZ(ObjSetU64(wrk, bo->fetch_objcore, OA_LEN, o));
 	VSB_destroy(&synth_body);
 	HSH_Unbusy(wrk, bo->fetch_objcore);
+	if (stale != NULL && bo->fetch_objcore->ttl > 0)
+		HSH_Kill(stale);
 	ObjSetState(wrk, bo->fetch_objcore, BOS_FINISHED);
 	return (F_STP_DONE);
 }
diff --git a/bin/varnishtest/tests/r01395.vtc b/bin/varnishtest/tests/r01395.vtc
index b7e388101..ce56cf708 100644
--- a/bin/varnishtest/tests/r01395.vtc
+++ b/bin/varnishtest/tests/r01395.vtc
@@ -1,4 +1,4 @@
-varnishtest "Test vcl_synth is called even if vcl_backend_fetch failed"
+varnishtest "Test vcl_backend_error is called if vcl_backend_fetch fails"
 
 varnish v1 -vcl {
 	backend default { .host = "${bad_backend}"; }
diff --git a/bin/varnishtest/tests/r02946.vtc b/bin/varnishtest/tests/r02946.vtc
new file mode 100644
index 000000000..b0eb3e6bf
--- /dev/null
+++ b/bin/varnishtest/tests/r02946.vtc
@@ -0,0 +1,45 @@
+varnishtest "#2946 - objcore leak for backend_synth"
+
+varnish v1 -vcl {
+	backend bad { .host = "${bad_backend}"; }
+
+	sub vcl_backend_error {
+		if (bereq.http.abandon) {
+			return (abandon);
+		}
+		if (bereq.http.ttl0) {
+			set beresp.ttl = 0s;
+			return (deliver);
+		}
+		set beresp.status = 200;
+		set beresp.ttl = 0.0001s;
+		set beresp.grace = 1h;
+		return (deliver);
+	}
+} -start
+
+client c1 -repeat 20 -keepalive {
+	txreq
+	rxresp
+	expect resp.status == 200
+
+	delay 0.001
+} -run
+
+delay 2
+
+client c1 {
+	txreq -hdr "abandon: true"
+	rxresp
+	expect resp.status == 200
+	expect resp.http.age > 1
+} -run
+
+client c1 {
+	txreq -hdr "ttl0: true"
+	rxresp
+	expect resp.status == 200
+	expect resp.http.age > 1
+} -run
+
+varnish v1 -expect n_objectcore == 1
diff --git a/doc/sphinx/users-guide/vcl-built-in-subs.rst b/doc/sphinx/users-guide/vcl-built-in-subs.rst
index a911ed005..0315b4838 100644
--- a/doc/sphinx/users-guide/vcl-built-in-subs.rst
+++ b/doc/sphinx/users-guide/vcl-built-in-subs.rst
@@ -495,8 +495,12 @@ vcl_backend_error
 This subroutine is called if we fail the backend fetch or if
 *max_retries* has been exceeded.
 
-A synthetic object is generated in VCL, whose body may be constructed
-using the ``synthetic()`` function.
+Returning with `abandon`_ does not leave a cache object.
+
+If returning with ``deliver`` and a ``beresp.ttl > 0s``, a synthetic
+cache object is generated in VCL, whose body may be constructed using
+the ``synthetic()`` function. This may be useful to increase the
+efficiency of failing backend requests.
 
 The `vcl_backend_error` subroutine may terminate with calling ``return()``
 with one of the following keywords: