varnish / varnish-modules

Collection of Varnish Cache modules (vmods) by Varnish Software
Other
185 stars 86 forks source link

Add check for block parameter in run_gc #207

Closed sumit-kushwah closed 1 year ago

sumit-kushwah commented 1 year ago

In this PR I have added a check for block parameter in run_gc function while removing expired buckets. We should keep blocked buckets until their block duration has ended, even if their expiration period has already ended.

I have added below condition in code.

if (now - x->last_used > x->period && (x->block == 0 || now > x->block)) {
    VRBT_REMOVE(tbtree, buckets, x);
    FREE_OBJ(x);
}
  1. x->block == 0 condition check that bucket is not currently blocked.
  2. now > x->blockcondition check that if the current time is greater than the end time the bucket was blocked.

This PR fixes the issue #206

sumit-kushwah commented 1 year ago

@gquintard @slimhazard Please look into it. if this fixes the issue.

gquintard commented 1 year ago

Hi @sumit-kushwah, thank you, but I think you are going about this the wrong way: we should first have a test case that reproduces the issue, then fix the bug. My intuition tells me that it's indeed the cause, but we need solid proof before changing the code.

sumit-kushwah commented 1 year ago

OK @gquintard . Your concern seems reasonable. I will try to reproduce the bug and see this block parameter check is the only issue.

gquintard commented 1 year ago

hold on, I think I have a reproducer and a fix, I'll open a PR tonight