Re-Setup the response after rollback

Fixes #3083

to get there, also centralize req->resp setup for deliver and synth:

No semantic changes other than some reordering, which also fixes an odd
log line ordering as shown by the change to c00018.vtc

bin/varnishd/cache/cache_req_fsm.c

@@ -104,6 +104,96 @@ cnt_transport(struct worker *wrk, struct req *req)
  * Deliver an object to client
  */
 
+static int
+resp_setup_deliver(struct req *req)
+{
+	struct http *h;
+	struct objcore *oc;
+
+	CHECK_OBJ_NOTNULL(req, REQ_MAGIC);
+	oc = req->objcore;
+	CHECK_OBJ_NOTNULL(oc, OBJCORE_MAGIC);
+
+	h = req->resp;
+
+	HTTP_Setup(h, req->ws, req->vsl, SLT_RespMethod);
+
+	if (HTTP_Decode(h, ObjGetAttr(req->wrk, oc, OA_HEADERS, NULL)))
+		return (-1);
+
+	http_ForceField(h, HTTP_HDR_PROTO, "HTTP/1.1");
+
+	if (req->is_hit)
+		http_PrintfHeader(h, "X-Varnish: %u %u", VXID(req->vsl->wid),
+		    ObjGetXID(req->wrk, oc));
+	else
+		http_PrintfHeader(h, "X-Varnish: %u", VXID(req->vsl->wid));
+
+	/*
+	 * We base Age calculation upon the last timestamp taken during client
+	 * request processing. This gives some inaccuracy, but since Age is only
+	 * full second resolution that shouldn't matter. (Last request timestamp
+	 * could be a Start timestamp taken before the object entered into cache
+	 * leading to negative age. Truncate to zero in that case).
+	 */
+	http_PrintfHeader(h, "Age: %.0f",
+	    floor(fmax(0., req->t_prev - oc->t_origin)));
+
+	http_SetHeader(h, "Via: 1.1 varnish (Varnish/6.3)");
+
+	if (cache_param->http_gzip_support &&
+	    ObjCheckFlag(req->wrk, oc, OF_GZIPED) &&
+	    !RFC2616_Req_Gzip(req->http))
+		RFC2616_Weaken_Etag(h);
+
+	return (0);
+}
+
+static int
+resp_setup_synth(struct req *req)
+{
+	struct http *h;
+
+	CHECK_OBJ_NOTNULL(req, REQ_MAGIC);
+
+	h = req->resp;
+
+	HTTP_Setup(h, req->ws, req->vsl, SLT_RespMethod);
+
+	AZ(req->objcore);
+	http_PutResponse(h, "HTTP/1.1", req->err_code, req->err_reason);
+
+	http_TimeHeader(h, "Date: ", W_TIM_real(req->wrk));
+	http_SetHeader(h, "Server: Varnish");
+	http_PrintfHeader(h, "X-Varnish: %u",
+	    VXID(req->vsl->wid));
+
+	/*
+	 * For late 100-continue, we suggest to VCL to close the connection to
+	 * neither send a 100-continue nor drain-read the request. But VCL has
+	 * the option to veto by removing Connection: close
+	 */
+	if (req->want100cont)
+		http_SetHeader(h, "Connection: close");
+
+	return (0);
+}
+
+int
+Resp_Setup(struct req *req, unsigned method)
+{
+	switch (method) {
+	case VCL_MET_DELIVER:
+		return (resp_setup_deliver(req));
+	case VCL_MET_SYNTH:
+		return (resp_setup_synth(req));
+	default:
+		WRONG("vcl method");
+	}
+
+	return (0);
+}
+
 static enum req_fsm_nxt
 cnt_deliver(struct worker *wrk, struct req *req)
 {
@@ -119,41 +209,12 @@ cnt_deliver(struct worker *wrk, struct req *req)
 
 	ObjTouch(req->wrk, req->objcore, req->t_prev);
 
-	HTTP_Setup(req->resp, req->ws, req->vsl, SLT_RespMethod);
-	if (HTTP_Decode(req->resp,
-	    ObjGetAttr(req->wrk, req->objcore, OA_HEADERS, NULL))) {
+	if (resp_setup_deliver(req)) {
 		(void)HSH_DerefObjCore(wrk, &req->objcore, HSH_RUSH_POLICY);
 		req->err_code = 500;
 		req->req_step = R_STP_SYNTH;
 		return (REQ_FSM_MORE);
 	}
-	http_ForceField(req->resp, HTTP_HDR_PROTO, "HTTP/1.1");
-
-	if (req->is_hit)
-		http_PrintfHeader(req->resp,
-		    "X-Varnish: %u %u", VXID(req->vsl->wid),
-		    ObjGetXID(wrk, req->objcore));
-	else
-		http_PrintfHeader(req->resp,
-		    "X-Varnish: %u", VXID(req->vsl->wid));
-
-	/*
-	 * We base Age calculation upon the last timestamp taken during
-	 * client request processing. This gives some inaccuracy, but
-	 * since Age is only full second resolution that shouldn't
-	 * matter. (Last request timestamp could be a Start timestamp
-	 * taken before the object entered into cache leading to negative
-	 * age. Truncate to zero in that case).
-	 */
-	http_PrintfHeader(req->resp, "Age: %.0f",
-	    floor(fmax(0., req->t_prev - req->objcore->t_origin)));
-
-	http_SetHeader(req->resp, "Via: 1.1 varnish (Varnish/6.3)");
-
-	if (cache_param->http_gzip_support &&
-	    ObjCheckFlag(req->wrk, req->objcore, OF_GZIPED) &&
-	    !RFC2616_Req_Gzip(req->http))
-		RFC2616_Weaken_Etag(req->resp);
 
 	VCL_deliver_method(req->vcl, wrk, req, NULL, NULL);
 	VSLb_ts_req(req, "Process", W_TIM_real(wrk));
@@ -221,8 +282,6 @@ cnt_vclfail(const struct worker *wrk, struct req *req)
 static enum req_fsm_nxt
 cnt_synth(struct worker *wrk, struct req *req)
 {
-	struct http *h;
-	double now;
 	struct vsb *synth_body;
 	ssize_t sz, szl;
 	uint8_t *ptr;
@@ -235,27 +294,12 @@ cnt_synth(struct worker *wrk, struct req *req)
 
 	wrk->stats->s_synth++;
 
-	now = W_TIM_real(wrk);
-	VSLb_ts_req(req, "Process", now);
+	VSLb_ts_req(req, "Process", W_TIM_real(wrk));
 
 	if (req->err_code < 100)
 		req->err_code = 501;
 
-	HTTP_Setup(req->resp, req->ws, req->vsl, SLT_RespMethod);
-	h = req->resp;
-	http_TimeHeader(h, "Date: ", now);
-	http_SetHeader(h, "Server: Varnish");
-	http_PrintfHeader(req->resp, "X-Varnish: %u", VXID(req->vsl->wid));
-	http_PutResponse(h, "HTTP/1.1", req->err_code, req->err_reason);
-
-	/*
-	 * For late 100-continue, we suggest to VCL to close the connection to
-	 * neither send a 100-continue nor drain-read the request. But VCL has
-	 * the option to veto by removing Connection: close
-	 */
-	if (req->want100cont) {
-		http_SetHeader(h, "Connection: close");
-	}
+	(void) resp_setup_synth(req);
 
 	synth_body = VSB_new_auto();
 	AN(synth_body);
@@ -281,14 +325,14 @@ cnt_synth(struct worker *wrk, struct req *req)
 		 * XXX: Should we reset req->doclose = SC_VCL_FAILURE
 		 * XXX: If so, to what ?
 		 */
-		HTTP_Setup(h, req->ws, req->vsl, SLT_RespMethod);
+		HTTP_Setup(req->resp, req->ws, req->vsl, SLT_RespMethod);
 		VSB_destroy(&synth_body);
 		req->req_step = R_STP_RESTART;
 		return (REQ_FSM_MORE);
 	}
 	assert(wrk->handling == VCL_RET_DELIVER);
 
-	http_Unset(h, H_Content_Length);
+	http_Unset(req->resp, H_Content_Length);
 	http_PrintfHeader(req->resp, "Content-Length: %zd",
 	    VSB_len(synth_body));
 

bin/varnishd/cache/cache_varnishd.h

@@ -360,6 +360,8 @@ void VRB_Free(struct req *);
 
 /* cache_req_fsm.c [CNT] */
 
+int Resp_Setup(struct req *, unsigned);
+
 enum req_fsm_nxt {
 	REQ_FSM_MORE,
 	REQ_FSM_DONE,

bin/varnishd/cache/cache_vrt.c

@@ -747,6 +747,8 @@ VRT_Rollback(VRT_CTX, VCL_HTTP hp)
 	if (hp == ctx->http_req) {
 		CHECK_OBJ_NOTNULL(ctx->req, REQ_MAGIC);
 		Req_Rollback(ctx->req);
+		if ((ctx->method & (VCL_MET_DELIVER | VCL_MET_SYNTH)) != 0)
+			Resp_Setup(ctx->req, ctx->method);
 	} else if (hp == ctx->http_bereq) {
 		CHECK_OBJ_NOTNULL(ctx->bo, BUSYOBJ_MAGIC);
 		Bereq_Rollback(ctx->bo);

bin/varnishtest/tests/c00018.vtc

@@ -123,11 +123,13 @@ logexpect l1 -v v1 -g raw {
 	expect 0 1011	VCL_call        {^HASH$}
 	expect 0 1011	VCL_return      {^lookup$}
 	expect 0 1011	Timestamp       {^Process:}
-	expect 0 1011	RespHeader      {^Date:}
-	expect 0 1011	RespHeader      {^Server: Varnish$}
-	expect 0 1011	RespHeader      {^X-Varnish: 1011$}
 	expect 0 1011	RespProtocol    {^HTTP/1.1$}
 	expect 0 1011	RespStatus      {^405$}
+	expect 0 1011	RespReason      {^Method Not Allowed$}
+	# XXX dup RespReason
+	expect 1 1011	RespHeader      {^Date:}
+	expect 0 1011	RespHeader      {^Server: Varnish$}
+	expect 0 1011	RespHeader      {^X-Varnish: 1011$}
 
 } -start