[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