[master] bbd4c476b centralize cleanup after fetch errors
Nils Goroll
nils.goroll at uplex.de
Mon Oct 7 14:27:06 UTC 2019
commit bbd4c476bf0e2d9b7282f4dab59fa390d304a470
Author: Nils Goroll <nils.goroll at 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
diff --git a/bin/varnishd/cache/cache_fetch.c b/bin/varnishd/cache/cache_fetch.c
index f9cab4055..ec393a092 100644
--- a/bin/varnishd/cache/cache_fetch.c
+++ b/bin/varnishd/cache/cache_fetch.c
@@ -84,6 +84,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);
+}
+
/*--------------------------------------------------------------------
* Turn the beresp into a obj
*/
@@ -232,7 +246,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 */
@@ -307,7 +321,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);
+ vbf_cleanup(bo);
VSLb(bo->vsl, SLT_Error, "Body cannot be fetched");
assert(bo->director_state == DIR_S_NULL);
return (F_STP_ERROR);
@@ -380,7 +394,7 @@ vbf_stp_startfetch(struct worker *wrk, struct busyobj *bo)
VSLb(bo->vsl, SLT_Error,
"304 response but not conditional fetch");
bo->htc->doclose = SC_RX_BAD;
- VDI_Finish(bo);
+ vbf_cleanup(bo);
return (F_STP_ERROR);
}
}
@@ -390,7 +404,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);
+ vbf_cleanup(bo);
if (wrk->handling == VCL_RET_ERROR)
return (F_STP_ERROR);
else
@@ -400,8 +414,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);
+ vbf_cleanup(bo);
if (bo->retries++ < cache_param->max_retries)
return (F_STP_RETRY);
@@ -491,8 +504,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(bo);
+ vbf_cleanup(bo);
if (!bo->do_stream) {
assert(bo->fetch_objcore->boc->state < BOS_STREAM);
// XXX: doclose = ?
@@ -529,7 +541,7 @@ vbf_stp_fetch(struct worker *wrk, struct busyobj *bo)
if (bo->filter_list == NULL ||
VCL_StackVFP(bo->vfc, bo->vcl, bo->filter_list)) {
(bo)->htc->doclose = SC_OVERLOAD;
- VDI_Finish(bo);
+ vbf_cleanup(bo);
return (F_STP_ERROR);
}
@@ -541,15 +553,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);
+ 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);
+ vbf_cleanup(bo);
return (F_STP_ERROR);
}
@@ -591,7 +602,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));
@@ -604,10 +618,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);
-
ObjSetState(wrk, bo->fetch_objcore, BOS_FINISHED);
VSLb_ts_busyobj(bo, "BerespBody", W_TIM_real(wrk));
if (bo->stale_oc != NULL)
@@ -673,7 +683,7 @@ vbf_stp_condfetch(struct worker *wrk, struct busyobj *bo)
if (bo->stale_oc->flags & OC_F_FAILED)
(void)VFP_Error(bo->vfc, "Template object failed");
if (bo->vfc->failed) {
- VDI_Finish(bo);
+ vbf_cleanup(bo);
wrk->stats->fetch_failed++;
return (F_STP_FAIL);
}
More information about the varnish-commit
mailing list