Replace the VCL refcount by a self-desribing list

Instead of counting the references, the VCL keeps track of them with up
to 31 characters in the description. It is the VMOD's responsibility to
keep track of the opaque struct vclref * and provide a meaningful user-
friendly description.

bin/varnishd/cache/cache_vcl.c

@@ -62,10 +62,18 @@ struct vcl {
 	char			state[8];
 	char			*loaded_name;
 	unsigned		busy;
-	unsigned		refcount;
 	unsigned		discard;
 	const char		*temp;
 	VTAILQ_HEAD(,backend)	backend_list;
+	VTAILQ_HEAD(,vclref)	ref_list;
+};
+
+struct vclref {
+	unsigned		magic;
+#define VCLREF_MAGIC		0x47fb6848
+	const struct vcl	*vcl;
+	VTAILQ_ENTRY(vclref)	list;
+	char			desc[32];
 };
 
 /*
@@ -252,7 +260,7 @@ vcl_KillBackends(struct vcl *vcl)
 
 	CHECK_OBJ_NOTNULL(vcl, VCL_MAGIC);
 	AZ(vcl->busy);
-	AZ(vcl->refcount);
+	assert(VTAILQ_EMPTY(&vcl->ref_list));
 	while (1) {
 		be = VTAILQ_FIRST(&vcl->backend_list);
 		if (be == NULL)
@@ -375,40 +383,59 @@ VRT_count(VRT_CTX, unsigned u)
 		    ctx->vcl->conf->ref[u].line, ctx->vcl->conf->ref[u].pos);
 }
 
-void
-VRT_ref_vcl(VRT_CTX)
+struct vclref *
+VRT_ref_vcl(VRT_CTX, const char *desc)
 {
 	struct vcl *vcl;
+	struct vclref* ref;
 
 	ASSERT_CLI();
 	CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
+	AN(desc);
+	AN(*desc);
 
 	vcl = ctx->vcl;
 	CHECK_OBJ_NOTNULL(vcl, VCL_MAGIC);
 	xxxassert(vcl->temp == VCL_TEMP_WARM);
 
+	ALLOC_OBJ(ref, VCLREF_MAGIC);
+	AN(ref);
+	ref->vcl = vcl;
+	snprintf(ref->desc, sizeof ref->desc, "%s", desc);
+
 	Lck_Lock(&vcl_mtx);
-	vcl->refcount++;
+	VTAILQ_INSERT_TAIL(&vcl->ref_list, ref, list);
 	Lck_Unlock(&vcl_mtx);
+
+	return (ref);
 }
 
 void
-VRT_rel_vcl(VRT_CTX)
+VRT_rel_vcl(VRT_CTX, struct vclref **refp)
 {
 	struct vcl *vcl;
+	struct vclref *ref;
+
+	AN(refp);
+	ref = *refp;
+	*refp = NULL;
 
 	CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
+	CHECK_OBJ_NOTNULL(ref, VCLREF_MAGIC);
 
 	vcl = ctx->vcl;
 	CHECK_OBJ_NOTNULL(vcl, VCL_MAGIC);
+	assert(vcl == ref->vcl);
 	assert(vcl->temp == VCL_TEMP_WARM || vcl->temp == VCL_TEMP_BUSY ||
 	    vcl->temp == VCL_TEMP_COOLING);
 
 	Lck_Lock(&vcl_mtx);
-	assert(vcl->refcount > 0);
-	vcl->refcount--;
+	assert(!VTAILQ_EMPTY(&vcl->ref_list));
+	VTAILQ_REMOVE(&vcl->ref_list, ref, list);
 	/* No garbage collection here, for the same reasons as in VCL_Rel. */
 	Lck_Unlock(&vcl_mtx);
+
+	FREE_OBJ(ref);
 }
 
 /*--------------------------------------------------------------------*/
@@ -455,6 +482,23 @@ vcl_failsafe_event(VRT_CTX, enum vcl_event_e ev)
 		WRONG("A VMOD cannot fail USE, COLD or DISCARD events");
 }
 
+static void
+vcl_print_refs(VRT_CTX)
+{
+	struct vcl *vcl;
+	struct vclref *ref;
+
+	CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
+	CHECK_OBJ_NOTNULL(ctx->vcl, VCL_MAGIC);
+	AN(ctx->msg);
+	vcl = ctx->vcl;
+	VSB_printf(ctx->msg, "VCL %s is waiting for:", vcl->loaded_name);
+	Lck_Lock(&vcl_mtx);
+	VTAILQ_FOREACH(ref, &ctx->vcl->ref_list, list)
+		VSB_printf(ctx->msg, "\n\t- %s", ref->desc);
+	Lck_Unlock(&vcl_mtx);
+}
+
 static int
 vcl_set_state(VRT_CTX, const char *state)
 {
@@ -477,16 +521,17 @@ vcl_set_state(VRT_CTX, const char *state)
 		if (vcl->busy == 0 && (vcl->temp == VCL_TEMP_WARM ||
 		    vcl->temp == VCL_TEMP_BUSY)) {
 
-			vcl->temp = vcl->refcount ? VCL_TEMP_COOLING :
-			    VCL_TEMP_COLD;
+			vcl->temp = VTAILQ_EMPTY(&vcl->ref_list) ?
+			    VCL_TEMP_COLD : VCL_TEMP_COOLING;
 			vcl_failsafe_event(ctx, VCL_EVENT_COLD);
 			vcl_BackendEvent(vcl, VCL_EVENT_COLD);
 		}
 		else if (vcl->busy)
 			vcl->temp = VCL_TEMP_BUSY;
+		else if (VTAILQ_EMPTY(&vcl->ref_list))
+			vcl->temp = VCL_TEMP_COLD;
 		else
-			vcl->temp = vcl->refcount ? VCL_TEMP_COOLING :
-			    VCL_TEMP_COLD;
+			vcl->temp = VCL_TEMP_COOLING;
 		break;
 	case '1':
 		assert(vcl->temp != VCL_TEMP_WARM);
@@ -494,7 +539,11 @@ vcl_set_state(VRT_CTX, const char *state)
 		if (vcl->temp == VCL_TEMP_BUSY)
 			vcl->temp = VCL_TEMP_WARM;
 		/* The VCL must first reach a stable cold state */
-		else if (vcl->temp != VCL_TEMP_COOLING) {
+		else if (vcl->temp == VCL_TEMP_COOLING) {
+			vcl_print_refs(ctx);
+			i = -1;
+		}
+		else {
 			vcl->temp = VCL_TEMP_WARM;
 			i = vcl_setup_event(ctx, VCL_EVENT_WARM);
 			if (i == 0)
@@ -558,6 +607,7 @@ VCL_Load(struct cli *cli, const char *name, const char *fn, const char *state)
 	vcl->loaded_name = strdup(name);
 	XXXAN(vcl->loaded_name);
 	VTAILQ_INIT(&vcl->backend_list);
+	VTAILQ_INIT(&vcl->ref_list);
 
 	vcl->temp = VCL_TEMP_INIT;
 
@@ -609,7 +659,7 @@ VCL_Nuke(struct vcl *vcl)
 	assert(vcl != vcl_active);
 	assert(vcl->discard);
 	AZ(vcl->busy);
-	AZ(vcl->refcount);
+	assert(VTAILQ_EMPTY(&vcl->ref_list));
 	VTAILQ_REMOVE(&vcl_head, vcl, list);
 	ctx.method = VCL_MET_FINI;
 	ctx.handling = &hand;
@@ -726,7 +776,7 @@ ccf_config_discard(struct cli *cli, const char * const *av, void *priv)
 	vcl->discard = 1;
 	Lck_Unlock(&vcl_mtx);
 
-	if (vcl->busy == 0 && vcl->refcount == 0)
+	if (vcl->temp == VCL_TEMP_COLD)
 		VCL_Nuke(vcl);
 }
 

bin/varnishtest/tests/v00045.vtc

@@ -6,7 +6,7 @@ server s1 -start
 varnish v1 -vcl+backend {
 	import ${vmod_debug};
 	sub vcl_init {
-		debug.vcl_release_delay(2s);
+		debug.vcl_release_delay(3s);
 	}
 } -start
 
@@ -21,9 +21,19 @@ shell {
 	grep "auto/cooling.*vcl1" >/dev/null
 }
 
+# It can't be warmed up yet
+delay 1
+shell {
+	${varnishadm} -n ${v1_name} vcl.state vcl1 warm 2>/dev/null |
+	grep "vmod-debug ref on vcl1" >/dev/null
+}
+
 # It will eventually cool down
 delay 2
 shell {
 	${varnishadm} -n ${v1_name} vcl.list |
 	grep "auto/cold.*vcl1" >/dev/null
 }
+
+# At this point it becomes possible to warm up again
+varnish v1 -cliok "vcl.state vcl1 warm"

doc/sphinx/reference/vmod.rst

@@ -397,6 +397,28 @@ reference by calling ``VRT_ref_vcl`` when you receive a ``VCL_EVENT_WARM`` and
 later calling ``VRT_rel_vcl`` once the background job is over. Receiving a
 ``VCL_EVENT_COLD`` is your cue to terminate any background job bound to a VCL.
 
+You can find an example of VCL references in vmod-debug::
+
+	priv_vcl->vclref = VRT_ref_vcl(ctx, "vmod-debug");
+	...
+	VRT_rel_vcl(&ctx, &priv_vcl->vclref);
+
+In this simplified version, you can see that you need at least a VCL-bound data
+structure like a ``PRIV_VCL`` or a VMOD object to keep track of the reference
+and later release it. You also have to provide a description, it will be printed
+to the user if they try to warm up a cooling VCL::
+
+	$ varnishadm vcl.list
+	available  auto/cooling       0 vcl1
+	active     auto/warm          0 vcl2
+
+	$ varnishadm vcl.state vcl1 warm
+	Command failed with error code 300
+	Failed <vcl.state vcl1 auto>
+	Message:
+		VCL vcl1 is waiting for:
+		- vmod-debug
+
 In the case where properly releasing resources may take some time, you can
 opt for an asynchronous worker, either by spawning a thread and tracking it, or
 by using Varnish's worker pools.

include/vrt.h

@@ -295,8 +295,9 @@ struct vmod_priv {
 typedef int vmod_event_f(VRT_CTX, struct vmod_priv *, enum vcl_event_e);
 #endif
 
-void VRT_ref_vcl(VRT_CTX);
-void VRT_rel_vcl(VRT_CTX);
+struct vclref;
+struct vclref * VRT_ref_vcl(VRT_CTX, const char *);
+void VRT_rel_vcl(VRT_CTX, struct vclref **);
 
 void VRT_priv_fini(const struct vmod_priv *p);
 struct vmod_priv *VRT_priv_task(VRT_CTX, void *vmod_id);

lib/libvmod_debug/vmod_debug.c

@@ -45,6 +45,8 @@ struct priv_vcl {
 #define PRIV_VCL_MAGIC		0x8E62FA9D
 	char			*foo;
 	uintptr_t		exp_cb;
+	struct vcl		*vcl;
+	struct vclref		*vclref;
 };
 
 static VCL_DURATION vcl_release_delay = 0.0;
@@ -248,6 +250,8 @@ priv_vcl_free(void *priv)
 		EXP_Deregister_Callback(&priv_vcl->exp_cb);
 		VSL(SLT_Debug, 0, "exp_cb: deregistered");
 	}
+	AZ(priv_vcl->vcl);
+	AZ(priv_vcl->vclref);
 	FREE_OBJ(priv_vcl);
 	AZ(priv_vcl);
 }
@@ -273,8 +277,10 @@ event_load(VRT_CTX, struct vmod_priv *priv)
 }
 
 static int
-event_warm(VRT_CTX)
+event_warm(VRT_CTX, struct vmod_priv *priv)
 {
+	struct priv_vcl *priv_vcl;
+	char buf[32];
 
 	VSL(SLT_Debug, 0, "%s: VCL_EVENT_WARM", VCL_Name(ctx->vcl));
 
@@ -284,7 +290,13 @@ event_warm(VRT_CTX)
 		return (-1);
 	}
 
-	VRT_ref_vcl(ctx);
+	CAST_OBJ_NOTNULL(priv_vcl, priv->priv, PRIV_VCL_MAGIC);
+	AZ(priv_vcl->vcl);
+	AZ(priv_vcl->vclref);
+
+	snprintf(buf, sizeof buf, "vmod-debug ref on %s", VCL_Name(ctx->vcl));
+	priv_vcl->vcl = ctx->vcl;
+	priv_vcl->vclref = VRT_ref_vcl(ctx, buf);
 	return (0);
 }
 
@@ -292,29 +304,40 @@ static void*
 cooldown_thread(void *priv)
 {
 	struct vrt_ctx ctx;
+	struct priv_vcl *priv_vcl;
+
+	CAST_OBJ_NOTNULL(priv_vcl, priv, PRIV_VCL_MAGIC);
+	AN(priv_vcl->vcl);
+	AN(priv_vcl->vclref);
 
-	AN(priv);
 	INIT_OBJ(&ctx, VRT_CTX_MAGIC);
-	ctx.vcl = (struct vcl*)priv;
+	ctx.vcl = priv_vcl->vcl;
 
 	VTIM_sleep(vcl_release_delay);
-	VRT_rel_vcl(&ctx);
+	VRT_rel_vcl(&ctx, &priv_vcl->vclref);
+	priv_vcl->vcl = NULL;
 	return (NULL);
 }
 
 static int
-event_cold(VRT_CTX)
+event_cold(VRT_CTX, struct vmod_priv *priv)
 {
 	pthread_t thread;
+	struct priv_vcl *priv_vcl;
+
+	CAST_OBJ_NOTNULL(priv_vcl, priv->priv, PRIV_VCL_MAGIC);
+	AN(priv_vcl->vcl);
+	AN(priv_vcl->vclref);
 
 	VSL(SLT_Debug, 0, "%s: VCL_EVENT_COLD", VCL_Name(ctx->vcl));
 
 	if (vcl_release_delay == 0.0) {
-		VRT_rel_vcl(ctx);
+		VRT_rel_vcl(ctx, &priv_vcl->vclref);
+		priv_vcl->vcl = NULL;
 		return (0);
 	}
 
-	AZ(pthread_create(&thread, NULL, cooldown_thread, ctx->vcl));
+	AZ(pthread_create(&thread, NULL, cooldown_thread, priv_vcl));
 	AZ(pthread_detach(thread));
 	return (0);
 }
@@ -325,8 +348,8 @@ event_function(VRT_CTX, struct vmod_priv *priv, enum vcl_event_e e)
 
 	switch (e) {
 		case VCL_EVENT_LOAD: return event_load(ctx, priv);
-		case VCL_EVENT_COLD: return event_cold(ctx);
-		case VCL_EVENT_WARM: return event_warm(ctx);
+		case VCL_EVENT_WARM: return event_warm(ctx, priv);
+		case VCL_EVENT_COLD: return event_cold(ctx, priv);
 		default: return (0);
 	}
 }