std.cache_req_body(BYTES size, BOOL partial = 0)

[dridi]

trafficstars

Last week my employer organized among other things a hackathon day (thanks!) and I made this seemingly work. I revisited the code and reorganized the commits the next day during my flight, but didn't find much to say in an area of the code base not too familiar to me. I guess it's time to throw more eyeballs at it.

Quoting from the main patch of the series:

req.body: New BS_PARTIAL body status for caching

Partial caching allow VMODs to fetch and inspect part of the body. This gives the same properties as BS_CACHED, but allows the request body to be larger than what is stored. If a bereq send failures happens within the partial cached body delivery, it also becomes eligible to retries.

This enables a more robust caching and retry policy where not knowing the complete body size (chunked or h2 without content-length) or on the other hand knowing that a body is larger (content-length) does not lead to VCL failures.

It can also be valuable to inspect the beginning of a request body. For example an image upload service could parse images metadata directly in Varnish to enforce rules such as specific file formats or how large in pixels an image may be.

One limitation left on purpose is that BS_PARTIAL objects will be sent with chunked encoding to the backend, even when we know the length.

See individual commits for more details.

[nigoroll]

Because Transfer-Encoding: chunked is uncommon for request bodies, I would expect some backends to choke on it and would thus think a mode to foward the client's Content-Length would be good. On IRC, @bsdphk rightly pointed out the need for extra care and attention due to smuggling potential, and I agree that we should look into how to handle, for example, the case when a client fails to send the announced length.

[dridi]

bugwash: separate partial caching from request body status.

[dridi]

This morning I replaced https://github.com/varnishcache/varnish-cache/pull/3798/commits/c8ce29fc47e1a323335ee5252379b0b5c1ab0afa with 99f21e5c98...fc0a497200, then I added more coverage in the test case.

Basically:

• BS_PARTIAL is replaced by a req_body_cached flag
• another req_body_partial flag is added

[simonvik]

With the following test that i partly stole from vmod_bodyaccess I can assert varnish with "Condition((bo->req->body_oc) != 0) not true."

varnishtest "Assert"

server s1 {
    rxreq
    txresp -hdr "OK: yes"
    rxreq
    txresp -hdr "OK: yes"
} -start

varnish v1 -vcl+backend {
    import std;

    sub vcl_recv {
        if(req.method == "POST"){
                std.cache_req_body(10KB);
        }
    }

} -start

client c1 {
    txreq -req POST -body "BANANE"
    rxresp

    txreq
    rxresp
} -run

Not 100% sure why but someone probably has an idea

[dridi]

--- bin/varnishd/cache/cache_req_fsm.c
+++ bin/varnishd/cache/cache_req_fsm.c
@@ -893,6 +893,7 @@ cnt_recv_prep(struct req *req, const char *ci)
                req->hash_always_miss = 0;
                req->hash_ignore_busy = 0;
                req->hash_ignore_vary = 0;
+               req->req_body_cached = 0;
                req->client_identity = NULL;
                req->storage = NULL;
        }

We lack coverage of keepalive after a cached req body for HTTP/1.

[bsdphk]

How does this work with H2 ?

I guess it causes head-of-line-blocking, since I see nothing tweaking the H2 windows to stop the client from pushing ahead ?

Do we care ?

[dridi]

I don't think we need to care about h2, the rxbuf from #3661 is here to prevent head of line blocking.

FWIW, I plan to address the panic reported by @simonvik like this:

--- i/bin/varnishd/cache/cache_req_body.c
+++ w/bin/varnishd/cache/cache_req_body.c
@@ -125,6 +125,7 @@ vrb_pull(struct req *req, ssize_t maxsize, unsigned partial,
                AN(oa_len);
                req_bodybytes = oa_len;
                AZ(HSH_DerefObjCore(req->wrk, &req->body_oc, 0));
+               req->req_body_cached = 0;
                req->req_body_partial = 0;
        }
 
@@ -345,10 +346,14 @@ VRB_Free(struct req *req)
 
        CHECK_OBJ_NOTNULL(req, REQ_MAGIC);
 
-       if (req->body_oc == NULL)
+       AZ(req->req_body_partial);
+       if (req->body_oc == NULL) {
+               AZ(req->req_body_cached);
                return;
+       }
 
        r = HSH_DerefObjCore(req->wrk, &req->body_oc, 0);
+       req->req_body_cached = 0;
 
        // each busyobj may have gained a reference
        assert (r >= 0);
diff --git i/bin/varnishd/cache/cache_req_fsm.c w/bin/varnishd/cache/cache_req_fsm.c
index de2a4b8584..37840ccd14 100644
--- i/bin/varnishd/cache/cache_req_fsm.c
+++ w/bin/varnishd/cache/cache_req_fsm.c
@@ -895,6 +895,8 @@ cnt_recv_prep(struct req *req, const char *ci)
                req->hash_ignore_vary = 0;
                req->client_identity = NULL;
                req->storage = NULL;
+               AZ(req->req_body_cached);
+               AZ(req->req_body_partial);
        }
 
        req->is_hit = 0;

The idea is to clear the req_body_cached flag when req->body_oc's reference is dropped, something that used to be inherent to the assignment of req_body_status. I'll try to find some time to carefully review this aspect of the change.

[dridi]

Rebased against current master:

• flags are now cleared consistently and asserted
• test coverage added in 706844a18395eb9d8fdbab0eee018a157a17062b

[nigoroll]

24fe3772c520700b32711eb3b466dc1737327546 exposes that we have an issue with chunked encoding: When the body cache size exactly matches the length, we fail caching.

The trivial options are:

• ignore (and probably docfix) that, for chunked encoding, the buffer needs to be at least one byte larger than the actual length
• make the buffer one byte larger than specified

But this is only the symptom of another, possibly more relevant issue: the v1f chunked vfp currently can not send VFP_END with the last bytes. To solve this issue, we need to peek into the next chunk header before returning VFP_OK. See next comment...

[nigoroll]

I now have four branches in total with fixes and improvements around this PR. To reduce confusion, I will reference these in this single comment and delete some other references:

The mentions are in proposed order to be applied to trunk:

• #3811 is a small fix to V1F chunked processing which I had been asked to split from #3809
• #3809 optimizes V1F to signal VFP_END together with the last bytes and implements read-ahead to reduce the number of system calls
• https://github.com/nigoroll/varnish-cache/tree/partial_req_body is this PR with the two PRs above combined and one additional fix.
• https://github.com/nigoroll/varnish-cache/tree/partial_req_body_vrb_iterate2 adds VMOD access to partially cached request bodies
• https://github.com/nigoroll/varnish-cache/tree/partial_req_body_vrb_iterate2_req_filters adds req.filters