Make it possible to call VCL_vcl_rel() during object finalization

[slimhazard]

At the request of @bsdphk I'm posting this here as a reminder after a conversation on #varnish-hacking.

VCL_vcl_rel(VRT_CTX, VCL_VCL) requires the VRT_CTX argument, but no VRT_CTX argument is available in an object finalizer. So it can't be used (without some trickery) by a VMOD object that holds references to VCLs.

The implementation of VCL_vcl_rel actually doesn't use the ctx argument, other than calling CHECK_OBJ_NOTNULL on it. Decrementing the refcount on the VCL only depends on the VCL object, not on the ctx.

This is used here: https://code.uplex.de/uplex-varnish/libvmod-dispatch
And the call can actually be done in the object finalizer with this:

static struct vrt_ctx dummy_ctx = { .magic = VRT_CTX_MAGIC };

And then passing in &dummy_ctx in the VRT_CTX position. That satisfies CHECK_OBJ_NOTNULL, and then the VCL deref gets executed correctly, but of course it is quite hackish.

The options appear to be:

• Add a VRT_CTX argument to object finalizers.
• Remove the VRT_CTX arg from VCL_vcl_rel
• Handle all of this in a different way entirely.

[bsdphk]

The problem here is that the MGT process manages the VCL refcounts (too).

We can only allow a VCL(+ its vmods) to rel/ref VCL's VCC knew about and alerted MGT to.

[Dridi]

For VCL references held by VMODs, there is VRT_re[fl]_vcl already. This doesn't mess with the MGT sync and is reported in the CLI when it prevents a discard.

See 50aeefc for the patch, then 50aeefc and 502a871 for the test case.

This doesn't solve the null VRT_CTX problem, but this is the API that doesn't mess with the MGT.

[bsdphk]

I'm timing this ticket out.

If the issue is still relevant, at the very least provide a real-world example why it would be necessary for VMODs to hold on to other VCLs than their own.

[nigoroll]

@bsdphk the use case is documented here for long: 50aeefc#diff-28a38e034a15459ccc462fba6c3db5c1R386
One example of a vmod actually doing this is https://github.com/nigoroll/libvmod-dynamic/ which, unsurprisingly, was written by @Dridi
One place where the CTX hack is required is here: https://github.com/nigoroll/libvmod-dynamic/blob/master/src/vmod_dynamic.c#L618

Other than that, I think that having a CTX in object finalizes will have other benefits (for example, how would one log from a finalizer without a msg/vsl pointer?)

Regarding references to VCLs, I think that an alternative would be to wait for background tasks to finish in a destructor, but I could imagine this would create all kinds of vicious circles (e.g. if a backend needs a background task, which only finishes after its references are gone, without any extra complications we would immediately run into a dead lock)

[nigoroll]

bugwash: will see how the new api looks in practice, then close.
The broader scope for @slimhazard s use case in https://code.uplex.de/uplex-varnish/libvmod-dispatch needs more attention on top

[nigoroll]

@bsdphk Reviewing your change 43da3e4, I do not understand your intention.
We already had the following interfaces

refcounting via (struct vcl).nrefs

• VRT_ref_vcl() / VRT_rel_vcl() documented as intended for vmods to hold references here 50aeefc#diff-28a38e034a15459ccc462fba6c3db5c1R386
  • reference is registered via (struct vcl).ref_list - this is the only interface using ref_list
• VRT_vcl_get() now VPI_vcl_get() / VRT_vcl_rel() now VPI_vcl_rel()
  • no (struct vcl).ref_list

refcounting via (struct vcl).busy

• VCL_Ref() / VCL_Rel()
• vcl_get called from VCL_Refresh() and VPI_vcl_select()

now added:

• VRT_VCL_Busy() / VRT_VCL_Unbusy() wrapping VCL_Ref() / VCL_Rel(),

Regarding naming, I think if we touch this interface getting Get/Ref/Rel/Busy/Unbusy clear would help - right now ref/rel can refer to either busy or nrefs, as can get.

The way I read the code in https://github.com/varnishcache/varnish-cache/blob/master/bin/varnishd/cache/cache_vcl.c#L680 I suspect you might want to re-purpose nrefs for label references only, but, again, your intentions are unclear to me. Can you please explain?

Do you expect vmods to use the busy interface? Will VRT_ref_vcl() / VRT_rel_vcl() be deprecated?

[nigoroll]

Also, the original issue is not addressed: VRT_VCL_Busy() / VRT_VCL_Unbusy() also take a VRT_CTX, so fact that finalizers do not have a CTX is as much an issue as it was with VRT_ref_vcl() / VRT_rel_vcl()

[nigoroll]

Regarding the actual implementation, trying it here nigoroll/libvmod-dynamic@bd94be6 revealed the problem that the temperature lock is being held during VCL Events, so VCL_Ref() via VRT_VCL_Busy() would deadlock:

***  v1    0.4 debug|Assert error in VCL_Ref(), cache/cache_vrt_vcl.c line 116:
***  v1    0.4 debug|  Condition(((*__errno_location ())=pthread_rwlock_rdlock(&vcl->temp_rwl)) == 0) not true.
***  v1    0.4 debug|version = varnish-trunk revision 211f72da35816b5b2b8767bc0fe8c111f250cafe, vrt api = 9.0
***  v1    0.4 debug|ident = Linux,4.9.0-8-amd64,x86_64,-jnone,-sdefault,-sdefault,-hcritbit,epoll
***  v1    0.4 debug|now = 5172.119473 (mono), 1556174542.970345 (real)
***  v1    0.4 debug|Backtrace:
***  v1    0.4 debug|  0x559c68697386: varnishd(+0x4e386) [0x559c68697386]
***  v1    0.4 debug|  0x559c68700300: varnishd(VAS_Fail+0x40) [0x559c68700300]
***  v1    0.4 debug|  0x559c686b54d3: varnishd(VCL_Ref+0xf3) [0x559c686b54d3]
***  v1    0.4 debug|  0x7f59879f9bac: ./vmod_cache/_vmod_dynamic.70e767127892fa210428aff42dfd908ab267d4ddf9fc41fbc7f3c9dae7d58e20(vmod_event+0x17c) [0x7f59879f9bac]
***  v1    0.4 debug|  0x7f59889f91c4: vcl_vcl1.1556174542.778603/vgc.so(+0x31c4) [0x7f59889f91c4]
***  v1    0.4 debug|  0x559c686a61a4: varnishd(+0x5d1a4) [0x559c686a61a4]
***  v1    0.4 debug|  0x559c686a6504: varnishd(+0x5d504) [0x559c686a6504]
***  v1    0.4 debug|  0x559c686a8189: varnishd(+0x5f189) [0x559c686a8189]
***  v1    0.4 debug|  0x559c6870245a: varnishd(+0xb945a) [0x559c6870245a]
***  v1    0.4 debug|  0x559c6870291c: varnishd(VCLS_Poll+0x34c) [0x559c6870291c]
***  v1    0.4 debug|errno = 35 (Resource deadlock avoided)