Mailing List Archive

[6.0] 7b40b80d5 centralize cleanup after fetch errors
commit 7b40b80d573caace9c4da76230d4a0c04085d3fd
Author: Nils Goroll <nils.goroll@uplex.de>
Date: Wed Oct 2 16:43:05 2019 +0200

centralize cleanup after fetch errors

imples the following changes:

* VDI_Finish() is now always conditional on bo->director_state !=
DIR_S_NULL, making it idempotent

* introduces additional calls to VFP_Close() from startfetch and
for the filter_list / VCL_StackVFP error in vbf_stp_fetch(),
but VFP_Close() is idempotent.

* adds VFP_Close() for VFP_Open() failure in vbf_stp_fetch() which
I think is actually missing (for the case that some VFPs could
get opened before the open failure)

* calls VDI_Finish() earlier in vbf_stp_fetchend: I checked the
code and can not see any issue with this.

motivated by #3009

Conflicts:
bin/varnishd/cache/cache_fetch.c

diff --git a/bin/varnishd/cache/cache_fetch.c b/bin/varnishd/cache/cache_fetch.c
index bd02fc822..0c0d79c9d 100644
--- a/bin/varnishd/cache/cache_fetch.c
+++ b/bin/varnishd/cache/cache_fetch.c
@@ -85,6 +85,20 @@ vbf_allocobj(struct busyobj *bo, unsigned l)
return (STV_NewObject(bo->wrk, bo->fetch_objcore, stv_transient, l));
}

+static void
+vbf_cleanup(struct busyobj *bo)
+{
+ struct vfp_ctx *vfc;
+
+ CHECK_OBJ_NOTNULL(bo, BUSYOBJ_MAGIC);
+ vfc = bo->vfc;
+ CHECK_OBJ_NOTNULL(vfc, VFP_CTX_MAGIC);
+
+ VFP_Close(vfc);
+ if (bo->director_state != DIR_S_NULL)
+ VDI_Finish(bo->wrk, bo);
+}
+
/*--------------------------------------------------------------------
* Turn the beresp into a obj
*/
@@ -233,7 +247,7 @@ vbf_stp_retry(struct worker *wrk, struct busyobj *bo)

VSLb_ts_busyobj(bo, "Retry", W_TIM_real(wrk));

- /* VDI_Finish must have been called before */
+ /* VDI_Finish (via vbf_cleanup) must have been called before */
assert(bo->director_state == DIR_S_NULL);

/* reset other bo attributes - See VBO_GetBusyObj */
@@ -281,7 +295,7 @@ vbf_304_logic(struct busyobj *bo)
VSLb(bo->vsl, SLT_Error,
"304 response but not conditional fetch");
bo->htc->doclose = SC_RX_BAD;
- VDI_Finish(bo->wrk, bo);
+ vbf_cleanup(bo);
return (-1);
}
return (1);
@@ -344,7 +358,7 @@ vbf_stp_startfetch(struct worker *wrk, struct busyobj *bo)

if (bo->htc->body_status == BS_ERROR) {
bo->htc->doclose = SC_RX_BODY;
- VDI_Finish(bo->wrk, bo);
+ vbf_cleanup(bo);
VSLb(bo->vsl, SLT_Error, "Body cannot be fetched");
assert(bo->director_state == DIR_S_NULL);
return (F_STP_ERROR);
@@ -391,8 +405,7 @@ vbf_stp_startfetch(struct worker *wrk, struct busyobj *bo)
if (wrk->handling == VCL_RET_ABANDON || wrk->handling == VCL_RET_FAIL ||
wrk->handling == VCL_RET_ERROR) {
bo->htc->doclose = SC_RESP_CLOSE;
-
- VDI_Finish(bo->wrk, bo);
+ vbf_cleanup(bo);
if (wrk->handling == VCL_RET_ERROR)
return (F_STP_ERROR);
else
@@ -402,8 +415,7 @@ vbf_stp_startfetch(struct worker *wrk, struct busyobj *bo)
if (wrk->handling == VCL_RET_RETRY) {
if (bo->htc->body_status != BS_NONE)
bo->htc->doclose = SC_RESP_CLOSE;
- if (bo->director_state != DIR_S_NULL)
- VDI_Finish(bo->wrk, bo);
+ vbf_cleanup(bo);

if (bo->retries++ < cache_param->max_retries)
return (F_STP_RETRY);
@@ -493,8 +505,7 @@ vbf_stp_fetchbody(struct worker *wrk, struct busyobj *bo)
if (vfc->failed) {
(void)VFP_Error(vfc, "Fetch pipeline failed to process");
bo->htc->doclose = SC_RX_BODY;
- VFP_Close(vfc);
- VDI_Finish(wrk, bo);
+ vbf_cleanup(bo);
if (!bo->do_stream) {
assert(bo->fetch_objcore->boc->state < BOS_STREAM);
// XXX: doclose = ?
@@ -587,7 +598,7 @@ vbf_stp_fetch(struct worker *wrk, struct busyobj *bo)

if (vbf_figure_out_vfp(bo)) {
(bo)->htc->doclose = SC_OVERLOAD;
- VDI_Finish((bo)->wrk, bo);
+ vbf_cleanup(bo);
return (F_STP_ERROR);
}

@@ -599,15 +610,14 @@ vbf_stp_fetch(struct worker *wrk, struct busyobj *bo)
if (VFP_Open(bo->vfc)) {
(void)VFP_Error(bo->vfc, "Fetch pipeline failed to open");
bo->htc->doclose = SC_RX_BODY;
- VDI_Finish(bo->wrk, bo);
+ vbf_cleanup(bo);
return (F_STP_ERROR);
}

if (vbf_beresp2obj(bo)) {
(void)VFP_Error(bo->vfc, "Could not get storage");
bo->htc->doclose = SC_RX_BODY;
- VFP_Close(bo->vfc);
- VDI_Finish(bo->wrk, bo);
+ vbf_cleanup(bo);
return (F_STP_ERROR);
}

@@ -659,7 +669,10 @@ vbf_stp_fetchend(struct worker *wrk, struct busyobj *bo)
{

AZ(bo->vfc->failed);
- VFP_Close(bo->vfc);
+
+ /* Recycle the backend connection before setting BOS_FINISHED to
+ give predictable backend reuse behavior for varnishtest */
+ vbf_cleanup(bo);

AZ(ObjSetU64(wrk, bo->fetch_objcore, OA_LEN,
bo->fetch_objcore->boc->len_so_far));
@@ -672,10 +685,6 @@ vbf_stp_fetchend(struct worker *wrk, struct busyobj *bo)
HSH_Unbusy(wrk, bo->fetch_objcore);
}

- /* Recycle the backend connection before setting BOS_FINISHED to
- give predictable backend reuse behavior for varnishtest */
- VDI_Finish(bo->wrk, bo);
-
ObjSetState(wrk, bo->fetch_objcore, BOS_FINISHED);
VSLb_ts_busyobj(bo, "BerespBody", W_TIM_real(wrk));
if (bo->stale_oc != NULL)
@@ -775,7 +784,7 @@ vbf_stp_condfetch(struct worker *wrk, struct busyobj *bo)
(void)VFP_Error(bo->vfc, "Template object failed");

if (bo->vfc->failed) {
- VDI_Finish(bo->wrk, bo);
+ vbf_cleanup(bo);
wrk->stats->fetch_failed++;
return (F_STP_FAIL);
}
_______________________________________________
varnish-commit mailing list
varnish-commit@varnish-cache.org
https://www.varnish-cache.org/lists/mailman/listinfo/varnish-commit