Mailing List Archive

Hi Reza,

I like this a lot. Though I am not completely sure about the way the
scoping is expressed. It seems to me that the scoping is (mostly) implicit
from where the new keyword is placed. If it's in vcl_backend_*, the scope
is bereq. If it's in vcl_init the scope is global, and if it is in the
others the scope is req (except when it's top). This leaves top as the only
odd scope, and if we found some other way to deal with that special case,
we would not need the scope cast in most places making it prettier. Perhaps
only require the scope cast for (top)?

Also how the scoping tests are performed in the vcc compiler leaves things
to be desired. The first attached test case shows something that fails as a
compiler error, but that I believe should be perfectly valid VCL (and the
ordering is certainly something I've seen in plenty of messy user VCLs). In
the test the vcl_deliver method comes before the vcl_recv in the input
text, and the scoping then errors out on seeing the object in vcl_deliver
that is defined in vcl_recv. The execution of vcl_recv is always before
vcl_deliver, so the code isn't wrong, and would work just fine if it wasn't
for the use of the object. I assume that the same issue is also present for
vcl_init today. I believe we should address this, and could do so by
enforcing an order in which the VCL functions are compiled. So after having
read all of the source code, the compiler should in order do vcl_init,
vcl_recv ... vcl_deliver, vcl_synth, vcl_backend_*.

The 2nd attached test case demonstrates another concern on uninitialised
objects. This test case fails with an assertion in the debug vmod for
obvious reasons. My concern is that it is tempting to write VCL code like
this, and having the Varnish assert itself by a simple VCL mistake is bad.
(I do believe that similar issues exist for our current vcl_init-only
director objects, though it's less error prone because noone writes
if-clauses in vcl_init).

One way of looking at this issue is to say that the debug object used here
is in the wrong to assert when it's uninitialised, and that the
uninitialised state is a valid vmod object state. The vmods then need to
define correct behaviour for these cases (e.g. do nothing for the right
meaning of nothing, for this case I guess return the empty string). And we
should formally define this in the nonexistant vmod developer guide.

Another approach is to define that object initialisation can't fail, which
is more in-spirit with the VCL language. So make the VCC compiler give
compiler errors if it can't say for sure that the new statement for an
object has been executed before the object method invocation. E.g. an
object method can't be used in vcl_recv for an object that is created in
vcl_deliver. This would then drag with it new scopes for every curly
bracket opening inside of the VCL functions. So an object can be new'ed and
used inside the if-statement, but is out-of-scope outside of it.

If we do define that object initialisation can't fail, I would also like to
see a proper exception mechanism build in. So vmods can throw exceptions
that halt VCL execution and safely call the priv free callbacks to clean
up, before presenting an error. Details unclear.

Lastly I think we should have the calling scope limits as vmod metadata in
place before allowing this. So a vmod object that's only meant to be called
in a global scope can declare that in its vcc file.

So in conclusion I am concerned about the Pandora's box this is opening,
and there are building blocks that are missing today that should be
addressed first.

Martin

On Wed, 9 Nov 2016 at 23:13 Reza Naghibi <reza@varnish-software.com> wrote:

> This is another discussion for VIP9. The previous mailing list thread is
> linked below [0].
>
> This allows objects to live in req, req.top, and bereq scopes. Currently
> objects are global to the VCL and can only be defined in vcl_init.
>
> The driver for this is allowing for multiple VMOD objects to exist in a
> single request, each with their own attributes. There is a usecase where we
> need to make multiple HTTP requests from VCL to 3rd party services and then
> build multiple security related digests on several aspects of the request,
> response, and 3rd party services. So having proper objects here would make
> the VMOD based solution extremely clean and easy to understand.
>
> I have a branch ready for a PR, but it was requested to have another
> discussion. Branch is located here [1] and based on master, 5.0, as of last
> week. m00026.vtc shows how the VCL looks [2]. The goal would be to have
> this available to VMODs in the next major release, Q1 2017.
>
> I will briefly address several of the concerns that have been brought up:
>
> * Syntax is too verbose. I agree that the first iteration kind of went
> overboard with the syntax. Currently, the syntax requires a (scope) style
> cast when you define the object and thats it. Please see [2].
>
> * This implementation is completely backwards compatible with how objects
> are defined in 4.X and does not require VMOD code to change. Object VMODs
> will be immediately available to these new scopes and will still go thru
> the _init() _fini() lifecycle.
>
> * Conflicts with VIP1. This VIP is only concerned with exposing PRIV_TASK
> and PRIV_TOP into existing objects at the VCL level. I believe VIP1 is
> concerned with PRIV_* at the VMOD level. So I believe these are a bit
> orthogonal and independent.
>
> * VMOD safety. Im pretty confident that VMOD objects actually allow for
> higher levels of reference and memory safety because objects have explicit
> _init() and _fini() methods and are passed in a struct for tracking state.
> So if you wanted to reference objects from other objects, then you could
> implement your own ref counting algorithm ontop of __init(), __finish(),
> and the state (or a gc implementation or whatever algorithm you want). I
> think this argument is a bit arbitrary because if we look at something as
> simple as VCL_STRING, which all VMODs have access to via req/resp headers,
> if you were to reference that in a VMOD, it will be freed from under you
> and leave you with unsafe code and memory. So any kind of unsafe VMOD
> example probably extends to the entire VMOD universe and should not be
> limited to this one VIP. Safe coding practices used today in VMODs should
> not be abandoned in context of this VIP :)
>
> It might be more helpful to see the compiled VCL code interact with the
> VRT to understand how this PR will work. So the C code for m00026.vtc is
> located here [3]. Just grep for g0, r0, r1, t0, and b0 to better see how
> this is implemented.
>
> [0]
> https://www.varnish-cache.org/lists/pipermail/varnish-dev/2016-April/008906.html
>
> [1]
> https://github.com/varnishcache/varnish-cache/compare/master...rezan:feature/object_scopes_master
>
> [2]
> https://github.com/rezan/varnish-cache/blob/feature/object_scopes_master/bin/varnishtest/tests/m00026.vtc
>
> [3] https://gist.github.com/rezan/76f92f76d31ee2d2105501b63612db43
>
> --
> Reza Naghibi
> Varnish Software
> _______________________________________________
> varnish-dev mailing list
> varnish-dev@varnish-cache.org
> https://www.varnish-cache.org/lists/mailman/listinfo/varnish-dev

> scoping is (mostly) implicit

I agree here. Not sure why I have been fixated on this ornamental
prefixing/casting, but I can do away with it. (top) would still exist and
any future scopes that are invented which fall into req/bereq would also
have to be casted in this way.

> variable uses out of order

Right now this seems to be a limitation in the vcc. Im tempted to say this
would simply be irksome for the time being. I would have to research how
this could be addressed, but im feeling this fix is out of scope right now
(and can be fixed independently of this VIP).

> uninitialised objects

This is a very valid concern and good thing you brought this up. When I
first read it, I thought about how other languages tackle this, in
particular, Java and C. Both languages either error out or warn at compile
time about uninitialized variables. So I think we could use the same
approach. If you define a variable inside of a control statement (if/else),
then that's a compile error due to it possibly being uninitialized and
unsafe. So this definitely needs to be addressed and fixed.

There is also some weirdness around defining variables inside of custom
subs. This worked on 4.0, but I think something changed in 5.0. So this
needs to be fixed too :)

> proper exception mechanism

Totally agree, right now you can only assert and take down the entire
process. There are many times where it would be nice to just have a
'request panic' and kill the request, but leave everything else intact. Not
sure if this feature is in scope for this VIP, but its definitely something
that VMODs and Varnish code in general needs access to.

> scope limits as vmod metadata

Ya, I thought about this and it probably makes sense to add this in just to
give VMOD authors more assurances that their objects will be used in the
way they expect. In theory, it shouldn't matter, but obviously in practice
there is a big difference in VCL global vs request vs [insert random scope
here]. So I will see if I can address this as well with some new vcc
definition syntax.

This is good feedback, thank you.


--
Reza Naghibi
Varnish Software

On Fri, Nov 11, 2016 at 6:06 AM, Martin Blix Grydeland <
martin@varnish-software.com> wrote:

> Hi Reza,
>
> I like this a lot. Though I am not completely sure about the way the
> scoping is expressed. It seems to me that the scoping is (mostly) implicit
> from where the new keyword is placed. If it's in vcl_backend_*, the scope
> is bereq. If it's in vcl_init the scope is global, and if it is in the
> others the scope is req (except when it's top). This leaves top as the only
> odd scope, and if we found some other way to deal with that special case,
> we would not need the scope cast in most places making it prettier. Perhaps
> only require the scope cast for (top)?
>
> Also how the scoping tests are performed in the vcc compiler leaves things
> to be desired. The first attached test case shows something that fails as a
> compiler error, but that I believe should be perfectly valid VCL (and the
> ordering is certainly something I've seen in plenty of messy user VCLs). In
> the test the vcl_deliver method comes before the vcl_recv in the input
> text, and the scoping then errors out on seeing the object in vcl_deliver
> that is defined in vcl_recv. The execution of vcl_recv is always before
> vcl_deliver, so the code isn't wrong, and would work just fine if it wasn't
> for the use of the object. I assume that the same issue is also present for
> vcl_init today. I believe we should address this, and could do so by
> enforcing an order in which the VCL functions are compiled. So after having
> read all of the source code, the compiler should in order do vcl_init,
> vcl_recv ... vcl_deliver, vcl_synth, vcl_backend_*.
>
> The 2nd attached test case demonstrates another concern on uninitialised
> objects. This test case fails with an assertion in the debug vmod for
> obvious reasons. My concern is that it is tempting to write VCL code like
> this, and having the Varnish assert itself by a simple VCL mistake is bad.
> (I do believe that similar issues exist for our current vcl_init-only
> director objects, though it's less error prone because noone writes
> if-clauses in vcl_init).
>
> One way of looking at this issue is to say that the debug object used here
> is in the wrong to assert when it's uninitialised, and that the
> uninitialised state is a valid vmod object state. The vmods then need to
> define correct behaviour for these cases (e.g. do nothing for the right
> meaning of nothing, for this case I guess return the empty string). And we
> should formally define this in the nonexistant vmod developer guide.
>
> Another approach is to define that object initialisation can't fail, which
> is more in-spirit with the VCL language. So make the VCC compiler give
> compiler errors if it can't say for sure that the new statement for an
> object has been executed before the object method invocation. E.g. an
> object method can't be used in vcl_recv for an object that is created in
> vcl_deliver. This would then drag with it new scopes for every curly
> bracket opening inside of the VCL functions. So an object can be new'ed and
> used inside the if-statement, but is out-of-scope outside of it.
>
> If we do define that object initialisation can't fail, I would also like
> to see a proper exception mechanism build in. So vmods can throw exceptions
> that halt VCL execution and safely call the priv free callbacks to clean
> up, before presenting an error. Details unclear.
>
> Lastly I think we should have the calling scope limits as vmod metadata in
> place before allowing this. So a vmod object that's only meant to be called
> in a global scope can declare that in its vcc file.
>
> So in conclusion I am concerned about the Pandora's box this is opening,
> and there are building blocks that are missing today that should be
> addressed first.
>
> Martin
>
> On Wed, 9 Nov 2016 at 23:13 Reza Naghibi <reza@varnish-software.com>
> wrote:
>
>> This is another discussion for VIP9. The previous mailing list thread is
>> linked below [0].
>>
>> This allows objects to live in req, req.top, and bereq scopes. Currently
>> objects are global to the VCL and can only be defined in vcl_init.
>>
>> The driver for this is allowing for multiple VMOD objects to exist in a
>> single request, each with their own attributes. There is a usecase where we
>> need to make multiple HTTP requests from VCL to 3rd party services and then
>> build multiple security related digests on several aspects of the request,
>> response, and 3rd party services. So having proper objects here would make
>> the VMOD based solution extremely clean and easy to understand.
>>
>> I have a branch ready for a PR, but it was requested to have another
>> discussion. Branch is located here [1] and based on master, 5.0, as of last
>> week. m00026.vtc shows how the VCL looks [2]. The goal would be to have
>> this available to VMODs in the next major release, Q1 2017.
>>
>> I will briefly address several of the concerns that have been brought up:
>>
>> * Syntax is too verbose. I agree that the first iteration kind of went
>> overboard with the syntax. Currently, the syntax requires a (scope) style
>> cast when you define the object and thats it. Please see [2].
>>
>> * This implementation is completely backwards compatible with how objects
>> are defined in 4.X and does not require VMOD code to change. Object VMODs
>> will be immediately available to these new scopes and will still go thru
>> the _init() _fini() lifecycle.
>>
>> * Conflicts with VIP1. This VIP is only concerned with exposing PRIV_TASK
>> and PRIV_TOP into existing objects at the VCL level. I believe VIP1 is
>> concerned with PRIV_* at the VMOD level. So I believe these are a bit
>> orthogonal and independent.
>>
>> * VMOD safety. Im pretty confident that VMOD objects actually allow for
>> higher levels of reference and memory safety because objects have explicit
>> _init() and _fini() methods and are passed in a struct for tracking state.
>> So if you wanted to reference objects from other objects, then you could
>> implement your own ref counting algorithm ontop of __init(), __finish(),
>> and the state (or a gc implementation or whatever algorithm you want). I
>> think this argument is a bit arbitrary because if we look at something as
>> simple as VCL_STRING, which all VMODs have access to via req/resp headers,
>> if you were to reference that in a VMOD, it will be freed from under you
>> and leave you with unsafe code and memory. So any kind of unsafe VMOD
>> example probably extends to the entire VMOD universe and should not be
>> limited to this one VIP. Safe coding practices used today in VMODs should
>> not be abandoned in context of this VIP :)
>>
>> It might be more helpful to see the compiled VCL code interact with the
>> VRT to understand how this PR will work. So the C code for m00026.vtc is
>> located here [3]. Just grep for g0, r0, r1, t0, and b0 to better see how
>> this is implemented.
>>
>> [0] https://www.varnish-cache.org/lists/pipermail/varnish-dev/
>> 2016-April/008906.html
>>
>> [1] https://github.com/varnishcache/varnish-cache/compare/master...rezan:
>> feature/object_scopes_master
>>
>> [2] https://github.com/rezan/varnish-cache/blob/feature/
>> object_scopes_master/bin/varnishtest/tests/m00026.vtc
>>
>> [3] https://gist.github.com/rezan/76f92f76d31ee2d2105501b63612db43
>>
>> --
>> Reza Naghibi
>> Varnish Software
>> _______________________________________________
>> varnish-dev mailing list
>> varnish-dev@varnish-cache.org
>> https://www.varnish-cache.org/lists/mailman/listinfo/varnish-dev
>
>

On Fri, 11 Nov 2016 at 16:24 Reza Naghibi <reza@varnish-software.com> wrote:

> > variable uses out of order
>
> Right now this seems to be a limitation in the vcc. Im tempted to say this
> would simply be irksome for the time being. I would have to research how
> this could be addressed, but im feeling this fix is out of scope right now
> (and can be fixed independently of this VIP).
>

Agreed that it it can be fixed independently, though I do think that the
new object powers will make the problem more prevalent.


> > uninitialised objects
>
> This is a very valid concern and good thing you brought this up. When I
> first read it, I thought about how other languages tackle this, in
> particular, Java and C. Both languages either error out or warn at compile
> time about uninitialized variables. So I think we could use the same
> approach. If you define a variable inside of a control statement (if/else),
> then that's a compile error due to it possibly being uninitialized and
> unsafe. So this definitely needs to be addressed and fixed.
>

We do not correctly handle object initialization inside of control
statements today, so disallowing that at compile time seems like a natural
first step. Though there are use cases where it would be nice to be able to
do that, so having a plan for what it should be like down the line would
help to make sure we don't get it wrong in the mean time.

Martin