New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

std.cache_req_body() w/ return(pipe) is broken #1881

Closed
daghf opened this Issue Mar 15, 2016 · 2 comments

Comments

Projects
None yet
4 participants
@daghf
Member

daghf commented Mar 15, 2016

Expected Behavior

Varnish should pass the cached request body also in the event that the request was piped.

Current Behavior

The request body is drained when we do std.cache_req_body(), and since the busyobj for a piped request does not have a bo->req pointer set, it will not be able to recover the cached body.

Steps to Reproduce

The following test case fails after a timeout since the backend expects a request body that is never transmitted.

varnishtest "cache_req_body + pipe does not play well"

server s1 {
    rxreq
    expect req.bodylen == 6
    expect req.http.foo == "true"
    txresp
} -start

varnish v1 -vcl+backend {
    import std;

    sub vcl_recv {
        set req.http.foo = std.cache_req_body(10KB);
        return (pipe);
    }
} -start

varnish v1 -cliok "param.set debug +syncvsl"

client c1 {
    txreq -body "foobar"
    rxresp
    expect resp.status == 200
} -run

Your Environment

  • Version used: master
@fgsch

This comment has been minimized.

Show comment
Hide comment
@fgsch

fgsch Jun 14, 2016

Member

This fixes it. Haven't looked at the implications of this change though.

diff --git a/bin/varnishd/cache/cache_req_fsm.c b/bin/varnishd/cache/cache_req_fsm.c
index a2d830c..6e2478f 100644
--- a/bin/varnishd/cache/cache_req_fsm.c
+++ b/bin/varnishd/cache/cache_req_fsm.c
@@ -572,6 +572,10 @@ cnt_pipe(struct worker *wrk, struct req *req)
                nxt = REQ_FSM_MORE;
                break;
        case VCL_RET_PIPE:
+               AZ(bo->req);
+               AZ(bo->wrk);
+               bo->req = req;
+               bo->wrk = wrk;
                SES_Close(req->sp, VDI_Http1Pipe(req, bo));
                nxt = REQ_FSM_DONE;
                break;
Member

fgsch commented Jun 14, 2016

This fixes it. Haven't looked at the implications of this change though.

diff --git a/bin/varnishd/cache/cache_req_fsm.c b/bin/varnishd/cache/cache_req_fsm.c
index a2d830c..6e2478f 100644
--- a/bin/varnishd/cache/cache_req_fsm.c
+++ b/bin/varnishd/cache/cache_req_fsm.c
@@ -572,6 +572,10 @@ cnt_pipe(struct worker *wrk, struct req *req)
                nxt = REQ_FSM_MORE;
                break;
        case VCL_RET_PIPE:
+               AZ(bo->req);
+               AZ(bo->wrk);
+               bo->req = req;
+               bo->wrk = wrk;
                SES_Close(req->sp, VDI_Http1Pipe(req, bo));
                nxt = REQ_FSM_DONE;
                break;

@fgsch fgsch closed this in 0953ac1 Jul 5, 2016

hermunn added a commit that referenced this issue Aug 12, 2016

Make req reachable from the pipe code
We will need it if the body was consumed due to a call to
std.cache_req_body (VRB_Cache).

Analysis and test by daghf.

Fixes #1881.

Conflicts:
	bin/varnishd/cache/cache_req_fsm.c
@hermunn

This comment has been minimized.

Show comment
Hide comment
@hermunn

hermunn Aug 12, 2016

Contributor

Backport review: The commit 0953ac1 is backported as acc91cc.

Contributor

hermunn commented Aug 12, 2016

Backport review: The commit 0953ac1 is backported as acc91cc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment