For restarts, keep all req attributes except req.restarts and req.xid

Fixes #2405 for restarts
Merges #2447

remaining TODOs (but not the scope of this ticket):

* Keep bereq attributes for bereq
* improved rollback

bin/varnishd/cache/cache_req_fsm.c

@@ -722,7 +722,6 @@ cnt_restart(struct worker *wrk, struct req *req)
 	CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
 	CHECK_OBJ_NOTNULL(req, REQ_MAGIC);
 
-	req->director_hint = NULL;
 	if (++req->restarts > cache_param->max_restarts) {
 		VSLb(req->vsl, SLT_VCL_Error, "Too many restarts");
 		req->err_code = 503;
@@ -736,7 +735,6 @@ cnt_restart(struct worker *wrk, struct req *req)
 		req->err_code = 0;
 		req->req_step = R_STP_RECV;
 	}
-	req->is_hit = 0;
 	return (REQ_FSM_MORE);
 }
 
@@ -783,22 +781,23 @@ cnt_recv(struct worker *wrk, struct req *req)
 		} else {
 			http_PrintfHeader(req->http, "X-Forwarded-For: %s", ci);
 		}
-	}
+		http_CollectHdr(req->http, H_Cache_Control);
 
-	/* By default we use the first backend */
-	AZ(req->director_hint);
-	req->director_hint = VCL_DefaultDirector(req->vcl);
-	AN(req->director_hint);
+		/* By default we use the first backend */
+		AZ(req->director_hint);
+		req->director_hint = VCL_DefaultDirector(req->vcl);
+		AN(req->director_hint);
+
+		req->d_ttl = -1;
+		req->disable_esi = 0;
+		req->hash_always_miss = 0;
+		req->hash_ignore_busy = 0;
+		req->client_identity = NULL;
+		req->storage = NULL;
+	}
 
 	req->vdc->retval = 0;
-	req->d_ttl = -1;
-	req->disable_esi = 0;
-	req->hash_always_miss = 0;
-	req->hash_ignore_busy = 0;
-	req->client_identity = NULL;
-	req->storage = NULL;
-
-	http_CollectHdr(req->http, H_Cache_Control);
+	req->is_hit = 0;
 
 	if (req->req_body_status == REQ_BODY_FAIL) {
 		req->doclose = SC_OVERLOAD;

bin/varnishtest/tests/c00009.vtc

@@ -12,8 +12,20 @@ server s2 {
 	txresp -body "foobar"
 } -start
 
-varnish v1 -vcl+backend {
+varnish v1 -arg "-smysteve=malloc,1m" -vcl+backend {
 	sub vcl_recv {
+		if (req.restarts == 0) {
+			set req.url = "/foo";
+			set req.method = "POST";
+			set req.proto = "HTTP/1.2";
+			set req.http.preserveme = "1";
+			set req.storage = storage.mysteve;
+			set req.ttl = 42m;
+			set req.esi = false;
+			set req.backend_hint = s2;
+			set req.hash_ignore_busy = true;
+			set req.hash_always_miss = true;
+		}
 		set req.http.restarts = req.restarts;
 	}
 	sub vcl_backend_fetch {
@@ -35,14 +47,36 @@ varnish v1 -vcl+backend {
 		if (resp.status != 200) {
 			return (restart);
 		}
+		set resp.http.method = req.method;
+		set resp.http.url = req.url;
+		set resp.http.proto = req.proto;
+		set resp.http.preserveme = req.http.preserveme;
+		set resp.http.restarts = req.restarts;
+		set resp.http.storage = req.storage;
+		set resp.http.ttl = req.ttl;
+		set resp.http.esi = req.esi;
+		set resp.http.backend_hint = req.backend_hint;
+		set resp.http.hash-ignore-busy = req.hash_ignore_busy;
+		set resp.http.hash-always-miss = req.hash_always_miss;
 	}
 } -start
 
 client c1 {
-	txreq -url "/foo"
+	txreq -url "/"
 	rxresp
 	expect resp.status == 200
 	expect resp.bodylen == 6
+	expect resp.http.method == POST
+	expect resp.http.url == /foo
+	expect resp.http.proto == HTTP/1.2
+	expect resp.http.preserveme == 1
+	expect resp.http.restarts == 1
+	expect resp.http.storage == storage.mysteve
+	expect resp.http.ttl == 2520.000
+	expect resp.http.esi == false
+	expect resp.http.backend_hint == s2
+	expect resp.http.hash-ignore-busy == true
+	expect resp.http.hash-always-miss == true
 }
 
 client c1 -run

doc/changes.rst

@@ -6,6 +6,10 @@ Varnish Cache Trunk (ongoing)
   less than the number of allowed restarts, it now is the number of
   ``return(restart)`` calls per request.
 
+* Fix behaviour of restarts to how it was originally intended:
+  Restarts now leave all the request properties in place except for
+  ``req.restarts`` and ``req.xid``, which need to change by design.
+
 ================================
 Varnish Cache 5.2.0 (2017-09-15)
 ================================

doc/sphinx/users-guide/vcl-built-in-subs.rst

@@ -59,6 +59,10 @@ common return keywords
     parameter, control is passed to :ref:`vcl_synth` as for
     ``return(synth(503, "Too many restarts"))``
 
+    For a restart, all modifications to ``req`` attributes are
+    preserved except for ``req.restarts`` and ``req.xid``, which need
+    to change by design.
+
 -----------
 client side
 -----------

lib/libvcc/generate.py

@@ -330,8 +330,6 @@ sp_variables = [
 		or the director otherwise.
 		When used in string context, returns the name of the director
 		or backend, respectively.
-		Note: backend_hint gets reset to the default backend by
-		restarts!
 		"""
 	),
 	('req.hash_ignore_busy',