[master] 468a047 Expose is_hit to VCL as resp.is_hit

Federico Schwindt fgsch at lodoss.net
Wed Jan 7 01:51:06 CET 2015


Can we just do:

    return (ctx->req->is_hit ? ctx->req->objcore->hits : 0);

in VRT_r_obj_hits() and revert this commit altogether?

I'm not sure whether this was discussed but besides the macros now having
the wrong names (REQ_FLAG and VREQ[WR][01] used for other variables than
req) adding more knobs is not the way to go IMO.


On Wed, Jan 7, 2015 at 12:12 AM, Federico Schwindt <fgsch at lodoss.net> 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 Wed, Jan 7, 2015 at 12:10 AM, Federico Schwindt <fgsch at lodoss.net>
> 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 <nils.goroll at uplex.de> wrote:
>>
>>>
>>> commit 468a0472fa313dbff69269afc085957642011abc
>>> Author: Nils Goroll <nils.goroll at uplex.de>
>>> 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
>>> varnish-commit at varnish-cache.org
>>> https://www.varnish-cache.org/lists/mailman/listinfo/varnish-commit
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://www.varnish-cache.org/lists/pipermail/varnish-dev/attachments/20150107/448d019a/attachment-0001.html>


More information about the varnish-dev mailing list