[experimental-ims] 480c0a5 Go over the return(restart) code.

Poul-Henning Kamp phk at FreeBSD.org
Thu Dec 18 10:27:43 CET 2014


commit 480c0a5d0b7b11e8e6b66f90dd66a2d8b22d235f
Author: Poul-Henning Kamp <phk at FreeBSD.org>
Date:   Thu Apr 12 08:06:09 2012 +0000

    Go over the return(restart) code.
    
    This is vastly inspired by #1113 and scoof+martins testcase+patch.
    
    Add a new cnt* state "cnt_restart" to do the mandatory restart work,
    and to move the restart limit check out of vcl_recv{}, getting rid
    of a nasty XXX comment.
    
    Set the explicit 503 when over limit, reset in other cases.
    
    Expand scoof+martins test-case to cover more vcl methods, which
    were also affected, but not fixed in their patch.
    
    Fixes	#1113

diff --git a/bin/varnishd/cache/cache_center.c b/bin/varnishd/cache/cache_center.c
index 7104e0f..5530f1b 100644
--- a/bin/varnishd/cache/cache_center.c
+++ b/bin/varnishd/cache/cache_center.c
@@ -293,10 +293,8 @@ cnt_prepresp(struct sess *sp, struct worker *wrk, struct req *req)
 			(void)HSH_Deref(&wrk->stats, NULL, &req->obj);
 		}
 		AZ(req->obj);
-		req->restarts++;
-		req->director = NULL;
 		http_Teardown(req->resp);
-		sp->step = STP_RECV;
+		sp->step = STP_RESTART;
 		return (0);
 	default:
 		WRONG("Illegal action in vcl_deliver{}");
@@ -524,9 +522,7 @@ cnt_error(struct sess *sp, struct worker *wrk, struct req *req)
 	    req->restarts <  cache_param->max_restarts) {
 		HSH_Drop(wrk, &sp->req->obj);
 		VBO_DerefBusyObj(wrk, &req->busyobj);
-		req->director = NULL;
-		req->restarts++;
-		sp->step = STP_RECV;
+		sp->step = STP_RESTART;
 		return (0);
 	} else if (req->handling == VCL_RET_RESTART)
 		req->handling = VCL_RET_DELIVER;
@@ -660,8 +656,7 @@ cnt_fetch(struct sess *sp, struct worker *wrk, struct req *req)
 
 	switch (req->handling) {
 	case VCL_RET_RESTART:
-		req->restarts++;
-		sp->step = STP_RECV;
+		sp->step = STP_RESTART;
 		return (0);
 	case VCL_RET_ERROR:
 		sp->step = STP_ERROR;
@@ -1045,9 +1040,7 @@ cnt_hit(struct sess *sp, struct worker *wrk, struct req *req)
 		sp->step = STP_ERROR;
 		return (0);
 	case VCL_RET_RESTART:
-		req->director = NULL;
-		req->restarts++;
-		sp->step = STP_RECV;
+		sp->step = STP_RESTART;
 		return (0);
 	default:
 		WRONG("Illegal action in vcl_hit{}");
@@ -1231,9 +1224,7 @@ cnt_miss(struct sess *sp, struct worker *wrk, struct req *req)
 		sp->step = STP_PASS;
 		break;
 	case VCL_RET_RESTART:
-		req->restarts++;
-		req->director = NULL;
-		sp->step = STP_RECV;
+		sp->step = STP_RESTART;
 		break;
 	default:
 		WRONG("Illegal action in vcl_miss{}");
@@ -1350,6 +1341,37 @@ cnt_pipe(struct sess *sp, struct worker *wrk, struct req *req)
 }
 
 /*--------------------------------------------------------------------
+ *
+DOT subgraph xcluster_restart {
+DOT	restart [
+DOT		shape=record
+DOT		label="{cnt_restart}"
+DOT	]
+DOT }
+DOT RESTART -> restart [color=purple]
+DOT restart -> recv [color=purple]
+ */
+
+static int
+cnt_restart(struct sess *sp, const struct worker *wrk, struct req *req)
+{
+
+	CHECK_OBJ_NOTNULL(sp, SESS_MAGIC);
+	CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
+	CHECK_OBJ_NOTNULL(req, REQ_MAGIC);
+	
+	req->director = NULL;
+	if (++req->restarts >= cache_param->max_restarts) {
+		req->err_code = 503;
+		sp->step = STP_ERROR;
+	} else {
+		req->err_code = 0;
+		sp->step = STP_RECV;
+	}
+	return (0);
+}
+
+/*--------------------------------------------------------------------
  * RECV
  * We have a complete request, set everything up and start it.
  * We can come here both with a request from the client and with
@@ -1368,7 +1390,6 @@ DOT		label="{cnt_recv:|{vcl_hash\{\}|req.*}}"
 DOT	]
 DOT }
 DOT ESI_REQ [ shape=hexagon ]
-DOT RESTART -> recv [color=purple]
 DOT ESI_REQ -> recv
 DOT recv:pipe -> pipe [style=bold,color=orange]
 DOT recv:pass -> pass [style=bold,color=red]
@@ -1406,18 +1427,6 @@ cnt_recv(struct sess *sp, const struct worker *wrk, struct req *req)
 	VCL_recv_method(sp);
 	recv_handling = req->handling;
 
-	if (req->restarts >= cache_param->max_restarts) {
-		/*
-		 * XXX: Why not before vcl_recv{} ?  We go to vcl_error{}
-		 * XXX: without vcl_recv{} on 413 and 417 already.
-		 * XXX tell vcl_error why we come
-		 */
-		if (req->err_code == 0)
-			req->err_code = 503;
-		sp->step = STP_ERROR;
-		return (0);
-	}
-
 	if (cache_param->http_gzip_support &&
 	     (recv_handling != VCL_RET_PIPE) &&
 	     (recv_handling != VCL_RET_PASS)) {
diff --git a/bin/varnishtest/tests/r01113.vtc b/bin/varnishtest/tests/r01113.vtc
new file mode 100644
index 0000000..abcee83
--- /dev/null
+++ b/bin/varnishtest/tests/r01113.vtc
@@ -0,0 +1,62 @@
+varnishtest "HTTP status code when hitting max_restarts"
+
+server s1 {
+	rxreq
+	txresp
+	accept
+	rxreq
+	txresp
+	accept
+	rxreq
+	txresp
+	accept
+	rxreq
+	txresp
+} -start
+
+varnish v1 -vcl+backend {
+	sub vcl_hit {
+		if (req.url == "/hit") {
+			return(restart);
+		}
+	}
+	sub vcl_fetch {
+		if (req.url == "/fetch") {
+			return(restart);
+		}
+	}
+	sub vcl_deliver {
+		if (req.url == "/deliver") {
+			return(restart);
+		}
+	}
+	sub vcl_miss {
+		if (req.url == "/miss") {
+			return(restart);
+		}
+	}
+} -start -cliok "param.set max_restarts 1"
+
+client c1 {
+	txreq -url /hit
+	rxresp
+	expect resp.status == 200
+	txreq -url /hit
+	rxresp
+	expect resp.status == 503
+} -run
+client c1 {
+	txreq -url /miss
+	rxresp
+	expect resp.status == 503
+} -run
+client c1 {
+	txreq -url /fetch
+	rxresp
+	expect resp.status == 503
+} -run
+client c1 {
+	txreq -url /deliver
+	rxresp
+	expect resp.status == 503
+} -run
diff --git a/include/tbl/steps.h b/include/tbl/steps.h
index e6738cb..ee70874 100644
--- a/include/tbl/steps.h
+++ b/include/tbl/steps.h
@@ -31,6 +31,7 @@
 /*lint -save -e525 -e539 */
 STEP(wait,		WAIT,		(sp, sp->wrk, sp->req))
 STEP(first,		FIRST,		(sp, sp->wrk))
+STEP(restart,		RESTART,	(sp, sp->wrk, sp->req))
 STEP(recv,		RECV,		(sp, sp->wrk, sp->req))
 STEP(start,		START,		(sp, sp->wrk, sp->req))
 STEP(pipe,		PIPE,		(sp, sp->wrk, sp->req))



More information about the varnish-commit mailing list