I've changed your diff to avoid splitting rollback. I don't think we should
make that distinction.
Also added v_b_e to the test and renamed ws_snap to be more inline with
other variables. ymmv.
btw, your patches were 2/3 and 3/3. Is 1/3 missing?
On Mon, Jul 21, 2014 at 1:38 PM, Nils Goroll <[email protected]> wrote:
> I thought we probably won't want to bump the major version, so I kept
> rollback
> as a wrapper for rollback_req
>
> Fixes #1512
>
> _______________________________________________
> varnish-dev mailing list
> [email protected]
> https://www.varnish-cache.org/lists/mailman/listinfo/varnish-dev
>
bin/varnishd/cache/cache.h | 1 +
bin/varnishd/cache/cache_fetch.c | 9 ++++---
bin/varnishd/cache/cache_vrt.c | 14 +++++++---
bin/varnishtest/tests/c00069.vtc | 35 +++++++++++++++++++++++++
bin/varnishtest/tests/r01512.vtc | 56 ++++++++++++++++++++++++++++++++++++++++
5 files changed, 109 insertions(+), 6 deletions(-)
diff --git a/bin/varnishd/cache/cache.h b/bin/varnishd/cache/cache.h
index a6c7006..cc885a0 100644
--- a/bin/varnishd/cache/cache.h
+++ b/bin/varnishd/cache/cache.h
@@ -489,6 +489,7 @@ struct busyobj {
enum busyobj_state_e state;
struct ws ws[1];
+ char *ws_bo; /* WS for rollback */
struct vbc *vbc;
struct http *bereq0;
struct http *bereq;
diff --git a/bin/varnishd/cache/cache_fetch.c b/bin/varnishd/cache/cache_fetch.c
index 70efc36..fda7fa2 100644
--- a/bin/varnishd/cache/cache_fetch.c
+++ b/bin/varnishd/cache/cache_fetch.c
@@ -215,6 +215,12 @@ vbf_stp_mkbereq(const struct worker *wrk, struct busyobj *bo)
}
}
+ HTTP_Setup(bo->bereq, bo->ws, bo->vsl, SLT_BereqMethod);
+
+ bo->ws_bo = WS_Snapshot(bo->ws);
+
+ HTTP_Copy(bo->bereq, bo->bereq0);
+
VBO_setstate(bo, BOS_REQ_DONE);
return (F_STP_STARTFETCH);
}
@@ -269,9 +275,6 @@ vbf_stp_startfetch(struct worker *wrk, struct busyobj *bo)
else
AZ(bo->req);
- HTTP_Setup(bo->bereq, bo->ws, bo->vsl, SLT_BereqMethod);
- HTTP_Copy(bo->bereq, bo->bereq0);
-
http_PrintfHeader(bo->bereq,
"X-Varnish: %u", bo->vsl->wid & VSL_IDENTMASK);
diff --git a/bin/varnishd/cache/cache_vrt.c b/bin/varnishd/cache/cache_vrt.c
index 4038693..683dca3 100644
--- a/bin/varnishd/cache/cache_vrt.c
+++ b/bin/varnishd/cache/cache_vrt.c
@@ -361,9 +361,17 @@ VRT_Rollback(const struct vrt_ctx *ctx)
{
CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
- CHECK_OBJ_NOTNULL(ctx->req, REQ_MAGIC);
- HTTP_Copy(ctx->req->http, ctx->req->http0);
- WS_Reset(ctx->req->ws, ctx->req->ws_req);
+ if (ctx->method == VCL_MET_BACKEND_RESPONSE ||
+ ctx->method == VCL_MET_BACKEND_ERROR) {
+ CHECK_OBJ_NOTNULL(ctx->bo, BUSYOBJ_MAGIC);
+ HTTP_Copy(ctx->bo->bereq, ctx->bo->bereq0);
+ WS_Reset(ctx->bo->bereq->ws, ctx->bo->ws_bo);
+ WS_Reset(ctx->bo->ws, ctx->bo->ws_bo);
+ } else {
+ CHECK_OBJ_NOTNULL(ctx->req, REQ_MAGIC);
+ HTTP_Copy(ctx->req->http, ctx->req->http0);
+ WS_Reset(ctx->req->ws, ctx->req->ws_req);
+ }
}
/*--------------------------------------------------------------------*/
diff --git a/bin/varnishtest/tests/c00069.vtc b/bin/varnishtest/tests/c00069.vtc
new file mode 100644
index 0000000..9430e1d
--- /dev/null
+++ b/bin/varnishtest/tests/c00069.vtc
@@ -0,0 +1,35 @@
+varnishtest "Test Rollback_bereq"
+
+
+server s1 {
+ rxreq
+ expect req.url == "/foo"
+ expect req.http.foobar == "harck-coff"
+ txresp -status 400
+ accept
+ rxreq
+ expect req.url == "/bar"
+ expect req.http.foobar == "snark"
+ txresp -bodylen 5
+} -start
+
+varnish v1 -vcl+backend {
+ sub vcl_backend_fetch {
+ if (bereq.url == "/foo") {
+ set bereq.http.foobar = "harck-coff";
+ }
+ }
+
+ sub vcl_backend_response {
+ if (beresp.status == 400) {
+ rollback;
+ set bereq.url = "/bar";
+ return (retry);
+ }
+ }
+} -start
+
+client c1 {
+ txreq -url "/foo" -hdr "foobar: snark"
+ rxresp
+} -run
diff --git a/bin/varnishtest/tests/r01512.vtc b/bin/varnishtest/tests/r01512.vtc
new file mode 100644
index 0000000..19a6129
--- /dev/null
+++ b/bin/varnishtest/tests/r01512.vtc
@@ -0,0 +1,56 @@
+varnishtest "Regression test for #1512"
+
+# First test bereq changes across v_b_r and v_b_f
+
+server s1 {
+ rxreq
+ txresp -status 700
+} -start
+
+varnish v1 -vcl+backend {
+ sub vcl_backend_fetch {
+ if (bereq.http.x-abandon == "1") {
+ return (abandon);
+ }
+ }
+ sub vcl_backend_response {
+ if (beresp.status == 700) {
+ set bereq.http.x-abandon = "1";
+ return (retry);
+ }
+ }
+ sub vcl_synth {
+ set resp.status = 701;
+ }
+} -start
+
+client c1 {
+ txreq
+ rxresp
+ expect resp.status == 701
+} -run
+
+# Then across v_b_e and v_b_f
+
+varnish v1 -vcl {
+ backend bad { .host = "${bad_ip}"; }
+ sub vcl_backend_fetch {
+ set bereq.backend = bad;
+ if (bereq.http.x-abandon == "1") {
+ return (abandon);
+ }
+ }
+ sub vcl_backend_error {
+ set bereq.http.x-abandon = "1";
+ return (retry);
+ }
+ sub vcl_synth {
+ set resp.status = 702;
+ }
+}
+
+client c1 {
+ txreq
+ rxresp
+ expect resp.status == 702
+} -run
_______________________________________________
varnish-dev mailing list
[email protected]
https://www.varnish-cache.org/lists/mailman/listinfo/varnish-dev
