I must say I'm not very thrilled about having another variable to check whether it was a hit.
How much of an issue this really is and can we fix the code so we still use obj.hits? On Wed, Jan 7, 2015 at 12:10 AM, Federico Schwindt <[email protected]> wrote: > I must say I'm not very thrilled about having another variable to check > whether it was a hit. > > How much of an issue this really is and can we fix the code so we still > use obj.hits? > > On Tue, Jan 6, 2015 at 9:41 PM, Nils Goroll <[email protected]> wrote: > >> >> commit 468a0472fa313dbff69269afc085957642011abc >> Author: Nils Goroll <[email protected]> >> Date: Tue Jan 6 22:24:21 2015 +0100 >> >> Expose is_hit to VCL as resp.is_hit >> >> The common method to check obj.hits is racy: While the update to >> the hits counter happens under a lock, a hit could have happened >> before the missing request gets to read obj.hits. >> >> diff --git a/bin/varnishd/cache/cache.h b/bin/varnishd/cache/cache.h >> index 2e7fa47..82bf5ed 100644 >> --- a/bin/varnishd/cache/cache.h >> +++ b/bin/varnishd/cache/cache.h >> @@ -540,7 +540,7 @@ struct req { >> int restarts; >> int esi_level; >> >> -#define REQ_FLAG(l, r, w, d) unsigned l:1; >> +#define REQ_FLAG(l, o, r, w, d) unsigned l:1; >> #include "tbl/req_flags.h" >> #undef REQ_FLAG >> >> diff --git a/bin/varnishd/cache/cache_panic.c >> b/bin/varnishd/cache/cache_panic.c >> index d45458e..9bd99a7 100644 >> --- a/bin/varnishd/cache/cache_panic.c >> +++ b/bin/varnishd/cache/cache_panic.c >> @@ -415,7 +415,7 @@ pan_req(const struct req *req) >> } >> >> VSB_printf(pan_vsp, " flags = {\n"); >> -#define REQ_FLAG(l, r, w, d) if(req->l) VSB_printf(pan_vsp, " " #l >> ",\n"); >> +#define REQ_FLAG(l, o, r, w, d) if(req->l) VSB_printf(pan_vsp, " " #l >> ",\n"); >> #include "tbl/req_flags.h" >> #undef REQ_FLAG >> VSB_printf(pan_vsp, " }\n"); >> diff --git a/bin/varnishd/cache/cache_vrt_var.c >> b/bin/varnishd/cache/cache_vrt_var.c >> index 4c6f2c6..5b2e5c2 100644 >> --- a/bin/varnishd/cache/cache_vrt_var.c >> +++ b/bin/varnishd/cache/cache_vrt_var.c >> @@ -566,29 +566,29 @@ VRT_r_bereq_xid(VRT_CTX) >> * req fields >> */ >> >> -#define VREQW0(field) >> -#define VREQW1(field) \ >> +#define VREQW0(obj, field) >> +#define VREQW1(obj, field) \ >> void \ >> -VRT_l_req_##field(VRT_CTX, unsigned a) \ >> +VRT_l_##obj##_##field(VRT_CTX, unsigned a) \ >> { \ >> CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC); \ >> CHECK_OBJ_NOTNULL(ctx->req, REQ_MAGIC); \ >> ctx->req->field = a ? 1 : 0; \ >> } >> >> -#define VREQR0(field) >> -#define VREQR1(field) \ >> +#define VREQR0(obj, field) >> +#define VREQR1(obj, field) \ >> unsigned \ >> -VRT_r_req_##field(VRT_CTX) \ >> +VRT_r_##obj##_##field(VRT_CTX) \ >> { \ >> CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC); \ >> CHECK_OBJ_NOTNULL(ctx->req, REQ_MAGIC); \ >> return (ctx->req->field); \ >> } >> >> -#define REQ_FLAG(l, r, w, d) \ >> - VREQR##r(l) \ >> - VREQW##w(l) >> +#define REQ_FLAG(l, o, r, w, d) \ >> + VREQR##r(o, l) \ >> + VREQW##w(o, l) >> #include "tbl/req_flags.h" >> #undef REQ_FLAG >> >> diff --git a/bin/varnishtest/tests/v00013.vtc >> b/bin/varnishtest/tests/v00013.vtc >> index c428add..19e9f26 100644 >> --- a/bin/varnishtest/tests/v00013.vtc >> +++ b/bin/varnishtest/tests/v00013.vtc >> @@ -13,6 +13,11 @@ varnish v1 -vcl+backend { >> >> sub vcl_deliver { >> set resp.http.foo = obj.hits; >> + if (resp.is_hit) { >> + set resp.http.X-Hit = "yes"; >> + } else { >> + set resp.http.X-Hit = "no"; >> + } >> } >> } -start >> >> @@ -21,16 +26,19 @@ client c1 { >> rxresp >> expect resp.status == 200 >> expect resp.http.foo == 0 >> + expect resp.http.X-hit == "no" >> >> txreq >> rxresp >> expect resp.status == 200 >> expect resp.http.foo == 1 >> + expect resp.http.X-hit == "yes" >> >> txreq -url /foo >> rxresp >> expect resp.status == 200 >> expect resp.http.foo == 0 >> + expect resp.http.X-hit == "no" >> delay .1 >> >> txreq >> @@ -38,4 +46,5 @@ client c1 { >> expect resp.status == 200 >> expect resp.http.foo == 2 >> expect resp.http.bar >= 0.100 >> + expect resp.http.X-hit == "yes" >> } -run >> diff --git a/bin/varnishtest/tests/v00039.vtc >> b/bin/varnishtest/tests/v00039.vtc >> index 029e114..c248aa8 100644 >> --- a/bin/varnishtest/tests/v00039.vtc >> +++ b/bin/varnishtest/tests/v00039.vtc >> @@ -13,6 +13,11 @@ varnish v1 \ >> -vcl+backend { >> sub vcl_deliver { >> set resp.http.hits = obj.hits; >> + if (resp.is_hit) { >> + set resp.http.X-Hit = "yes"; >> + } else { >> + set resp.http.X-Hit = "no"; >> + } >> } >> } -start >> >> @@ -23,6 +28,7 @@ client c1 { >> expect resp.status == 200 >> expect resp.bodylen == 6 >> expect resp.http.hits == 0 >> + expect resp.http.X-Hit == "no" >> >> # This is a hit -> hits == 1 >> txreq -url "/" -hdr "Bar: 1" >> @@ -30,6 +36,7 @@ client c1 { >> expect resp.status == 200 >> expect resp.bodylen == 6 >> expect resp.http.hits == 1 >> + expect resp.http.X-Hit == "yes" >> >> # This is a miss on different vary -> hits == 0 >> txreq -url "/" -hdr "Bar: 2" >> @@ -37,6 +44,7 @@ client c1 { >> expect resp.status == 200 >> expect resp.bodylen == 4 >> expect resp.http.hits == 0 >> + expect resp.http.X-Hit == "no" >> >> # This is a hit -> hits == 1 >> txreq -url "/" -hdr "Bar: 2" >> @@ -44,6 +52,7 @@ client c1 { >> expect resp.status == 200 >> expect resp.bodylen == 4 >> expect resp.http.hits == 1 >> + expect resp.http.X-Hit == "yes" >> >> } -run >> >> diff --git a/include/tbl/req_flags.h b/include/tbl/req_flags.h >> index 6757d90..55f20f7 100644 >> --- a/include/tbl/req_flags.h >> +++ b/include/tbl/req_flags.h >> @@ -29,11 +29,11 @@ >> >> /*lint -save -e525 -e539 */ >> >> -/* lower, vcl_r, vcl_w, doc */ >> -REQ_FLAG(disable_esi, 0, 0, "") >> -REQ_FLAG(hash_ignore_busy, 1, 1, "") >> -REQ_FLAG(hash_always_miss, 1, 1, "") >> -REQ_FLAG(is_hit, 0, 0, "") >> -REQ_FLAG(wantbody, 0, 0, "") >> -REQ_FLAG(gzip_resp, 0, 0, "") >> +/* lower, vcl_obj, vcl_r, vcl_w, doc */ >> +REQ_FLAG(disable_esi, none, 0, 0, "") >> +REQ_FLAG(hash_ignore_busy, req, 1, 1, "") >> +REQ_FLAG(hash_always_miss, req, 1, 1, "") >> +REQ_FLAG(is_hit, resp, 1, 0, "") >> +REQ_FLAG(wantbody, none, 0, 0, "") >> +REQ_FLAG(gzip_resp, none, 0, 0, "") >> /*lint -restore */ >> diff --git a/lib/libvcc/generate.py b/lib/libvcc/generate.py >> index e582c0f..699bab3 100755 >> --- a/lib/libvcc/generate.py >> +++ b/lib/libvcc/generate.py >> @@ -564,8 +564,10 @@ sp_variables = [ >> 'INT', >> ( 'hit', 'deliver',), >> ( ), """ >> - The count of cache-hits on this object. A value of 0 >> indicates a >> - cache miss. >> + The count of cache-hits on this object. >> + >> + NB: obj.hits is not a reliable way to determine cache >> + misses. See resp.is_hit. >> """ >> ), >> ('obj.http.', >> @@ -645,6 +647,13 @@ sp_variables = [ >> The corresponding HTTP header. >> """ >> ), >> + ('resp.is_hit', >> + 'BOOL', >> + ( 'deliver',), >> + ( ), """ >> + If this response is from a cache hit. >> + """ >> + ), >> ('now', >> 'TIME', >> ( 'all',), >> >> _______________________________________________ >> varnish-commit mailing list >> [email protected] >> https://www.varnish-cache.org/lists/mailman/listinfo/varnish-commit >> > >
_______________________________________________ varnish-dev mailing list [email protected] https://www.varnish-cache.org/lists/mailman/listinfo/varnish-dev
