Drop common.Bind for goja native support

[mstoykov]

After dop251/goja@06e9948 the scratched out part of mine comment dop251/goja#243 (comment) is now possible 🎉 .

Whether we want to do is another question.

Summary of what currently common.Bind seems to be doing(probably missed something):
It's predominant roll is to make certain that for methods on the struct provided that look like:

func (s someStruct) something(ctx context.Context, another parameter int) (val, err) 

1. ctx will be set to the one provided as the argument to Bind
2. If the name starts with 'X' it will wrap it in a function call (removing the X from the name) so that it can be used as a constructor - this should probably be using goja.ConstructorCall
3. returning a map[string]interface{} with everything that was an exported property of the original object as name=>wrapper. Where sometimes the wrapper does nothing :).

The last point means that things such as using an interface in the arguments of a function, but common.Bind-ing the "to be" argument will lead to it not answering the interface{}. The code posted in the community forum showcases this a lot better, then I can put it in words 🤣 .

Other problems with the current code include that it is kind of ... hard to read due to the sheer amount of reflect in it. It also having support for *context.Context(the pointer part), which seems completely unnecessary.

Also needing to be called in order to have access to the context, is something that we will probably need to document for [xk6]*(https://github.com/k6io/xk6) and explain it.

Proposed change:

1. add the context.Context as a global object to each goja.Runtime, or something else so we can get it from the goja.Runtime
2. change all the calls to use

func(goja.FunctionCall, goja.Runtime) goja.Value

or for constructors - drop the 'X' and change to

func(goja.ConstructorCall, goja.Runtime) goja.Value

3. drop the common.Bind at all

This will mean that instead of having

func(ctx.Context, number int64) {
// do something with number
}

we will have something like

func(fc goja.FunctionCall, rt goja.Runtime) goja.Value {
   ctx = commong.GetRuntime(rt)
   number = fc.Argument(1).ToInteger()
   // do something with number
}

This is a lot closer to how JS works so it might not be such a big downside.

This will likely take more than 1 release cycle and will likely turn out to have other problems so I am not even certain if it's really worth it

[mei-rune]

https://github.com/runner-mei/gojs/blob/main/main.go

dop251/goja#243 (comment)

[mstoykov]

Hi @runner-mei, thanks for mentioning it, but what you are doing is what we are doing now, but without it being recursive. (which you probably know as you have our bind copied right next to this code ;))

The point of this issue is to drop all these additional calls and instead only set a global variable in goja that is our context and get it back from the Runtime provided in the call, without any additional code on our side and remembering that you need to call common.Bind, just pure goja.

Yes, we still need to call goja.Runtime.Set or goja.Runtime.ToValue , but having to remember that 9/10s of the calls are for goja.Runtime and the rest for our custom Runtime also seems like the kind of thing we don't want to do.

The biggest problem again (apart from the time it will take to do and the risk of breaking something which are probably bigger) is that we will not be able to have the arguments to the methods be with their concrete types instead we will be taking goja.FunctionCall. This seems like a good idea either way as we have more and more places where we need to take multiple types so this might be somewhat necessary either way.

[mstoykov]

Some findings while I was trying to drop the usage *context.Context everywhere and hopefully also drop it's support in Bind at a later point:
There is actually a reason that *context.Context is used that I will explain with the metrics module. Let look at
https://github.com/k6io/k6/blob/311b9055f8801baef2b1e0c12e3da8458c2e3c60/js/modules/k6/metrics/metrics.go#L67-L68
Here we are creating a new metric and are binding it to the context that is available at the time of New Counter invocation. Important parts:

1. New Counter is supposed to only be called in the init context
2. Counter.Add is supposed to be called only not in the init context
3. We bind to the context at the time we are in the init context and the way we "find out' whether we are in the init context is through looking for lib.State in the context.Context
   https://github.com/k6io/k6/blob/311b9055f8801baef2b1e0c12e3da8458c2e3c60/js/modules/k6/metrics/metrics.go#L72-L75

This means that the context that Bind actually gives to the Add function
https://github.com/k6io/k6/blob/311b9055f8801baef2b1e0c12e3da8458c2e3c60/js/common/bridge.go#L167-L181
that is the same context that is given to Bind at the time needs to somehow get updated with lib.State at a later point
Which is literally what happens here
https://github.com/k6io/k6/blob/311b9055f8801baef2b1e0c12e3da8458c2e3c60/js/runner.go#L474-L482
notice the last line (this code is also repeated for the handleSummary)
For the record VU.Context is actually a field from BundleInstance (which is embedded in VU) that gets set (as expected) during the initialization of the VU/Bundle, which means it is the same Context that gets Given to newMetric in this example and then the value that is pointed by the pointer is changed in the code above:
https://github.com/k6io/k6/blob/311b9055f8801baef2b1e0c12e3da8458c2e3c60/js/bundle.go#L242-L257

So what I am trying to say is that we can't drop the usage of *context.Context until we actually have another way to give an accurate context.Context instance to function calls ... or give them away for them to get it out of the Runtime ;)

[mstoykov]

Restating the problem:
Currently, we have to somehow give context.Context to js modules and then update that context.Context when we change stuff- move from init context to running VUs, running next iteration, running VU by different executors and so on.

Technically this is not limited to context.Context and to one thing. We do currently just throw everything on top of the context.Context but this is also not a requirement and below I will sometimes talk about "the thing" as an abstract 1 or more things that we need to provide js modules and abstract away from the fact it is context.Context.

Also, most js modules don't care about the context.Context but about lib.State or even more accurately about the things we have put on lib.State as lib.State is just the way we transfer it currently as it's easy and we just keep throwing things in it and ... then it turns out it's not available where we need it ... Or goja.Runtime

The current solution and its problems:

The current solution: Is for whatever will need access to this to call common.Bind with a pointer to the context. Thie automatically happens for the registered module so it can get the context.Context and then call common.Bind on any structs it returns that need to have access

• pros:
  • it is already what we have and it seems to have worked so far
  • it lets you write func(context.Context, int64, string) (string, error) and the common.Bind code will make certain to cast from and to the goja.Value/goja.FunctionCall
• cons:
  • you need to know that you need to call common.Bind and be really surprised when you haven't and it doesn't work
  • the common.Bind code is a huge ball of reflect
  • js extensions have a pointer to the context and can change it
  • we in general just throw everything on context or even worse lib.State which likely is not a very good idea
  • common.Bind does not return the same object so if you want to have access to the original object we will need common.Bind to provide a way somehow.

###Original Proposal for the fix:

For all intents and purposes, what we care about is some way to get to the thing (in this case context.Context) so we just need a mechanism for that. Goja provides us with access to goja.Runtime if the function calls have the signature
func(goja.FunctionCall, goja.Runtime) goja.Value at which point we can have the thing set in the goja.Runtime as global or multiple globals 🤷 :

• pros:
  • uses standard goja to do it
  • in a lot of the k6 modules cases all we care about is .... the goja.Runtime which means that this will save some code on that front.
  • the object isn't changed/wrapped so you can have access to the original one without that big of a problem
  • No XSomething hack to say it is a constructor ... you can use func(goja.ConstructorCall, goja.Runtime) goja.Object
• cons:
  • you need to "manually" cast from and to goja.Value and work with goja.FunctionCall, which also arguably is sometimes a requirement for the code but this will force you.
  • it uses globals and as we know they are the root of all evil /s
  • we might need to come up with some mechanism to not allow modification or at all access of this from js ?!?
    This definitely should be something like a function GetThing that returns you the thing value at the time of the call . Behind the scene it can still be just dereferencing the pointer to context.Context but ... this will no longer be necessary ;)

Proposal 2:

Technically again ... all we care about is for the modules and their methods and objects(and their methods) created from it to have access to the thing ... somehow. And to its updated values

So in practice, we can just ... give it to the module at initialization (or shortly after) and call it a day.

• pros:
  • super simple, no magic, no globals
  • goja does convert to and from goja.Values as well and also support throwing an exception on returned error
• cons:
  • the js modules writer needs to thread this everywhere which especially if it turns out they need something at a later point in the development in some object that is created by an object created by ... . created by the module object it might be ... quite a lot of work :(

Proposal 2 variant 1:

We can just have an interface like HasModuleInstancePerVU and have HasSetContext .... it gets a bit tricky after we have to HasModuleInstancePerVU ... should we call it on the registered module or the one that is instantiated?

• pros:
  • very simple and additive on top of the current solution, possibly compatible with common.Bind
• cons:
  • if we decide that just giving everything context and then adding everything on to of context and start having many things it will need a method per each new Thing which will likely be a PITA for the extension developers to find out they now need to implement SetMyNewThing to get access to it.
  • users need to know that they need to implement it to get to the thing ... might also be confusing that some other type that implements it and is returned by a module method doesn't get the same method called 🤷‍♂️, but that it needs to get it from the module, especially given this is how common.Bind kind of makes things seem to work.

Proposal 2 variant 2:

given the above and the fact that, while we can just staple everything on the context it is probably not the "greatest" idea ever.
we can have something like

// package js/modules
type ModuleInterface{ // name bad
	GetThing() Thing
	GetOtherThing() OtherThing
	secretCoolMethod() // probably something like `youNeedToEmbedTheProvidedModuleInterfaceInYourModuleNotIMplement This` will be a better name ... or maybe my java developer is peeking
}

type InnerModuleType struct{} // implement secretCoolMethod and everything else we need to provide to modules 

And then require the registering the module to have the signature New(ModuleInterface)ModuleInterface
So that a registering module will need to get our implementation and embed it in their implementation in order to get secretCoolMethod implemented.
(This apparently is being called sealed interface by some and is a pattern used by grpc among others for forward compatibility. I personally can't remember when I found out about this pattern, but it was before I refound it in the grpc package due to a change while updating it)

• pros:
  • this means that in the future we want to add a new method we can do it and have a default implementation that just works. For example, lately, we have talked about the fact that there is no way to find out k6 is starting or finishing execution so that extensions can do something about it. We probably shouldn't put this in lib.State for example, and stapling it on the context.Context also seems strange to me.
  • we also can have methods that just return errors if not implemented or panic in the default implementations.
  • this can be embedded in all the other things and if it certainly needs a new thing it can have access to it without the thing being context and having 20 GetSomethingState methods associated with it
• cons:
  • I would argue that this pattern is a bit confusing, but again documentation will help and at least make certain that the confusion is early on and makes users go read the docs.
  • probably a bit harder (but not impossible) to make it work with common.Bind at least for a time
  • we require everybody to always have the Thing even if they don't need to, but to be honest it is probably a requirement for most non-trivial extensions and internal modules to the point that this seems like less of cons and more of pro. This still doesn't require anything returned to also embed it.

[na--]

I don't grok all of the problems, requirements and potential solutions here yet, but I just wanted to mention that we need the current scenario context in every call of a Go function in the k6 JS API, so that Go function (e.g. sleep()) knows when the iteration is interrupted prematurely. So the thing either needs to be a context or to have a context embedded, and between the two, it's more idiomatic for a context to embed data than for the reverse.

Other than that, in general, I'd personally prefer a solution which makes writing Go modules as simple as possible, even if that means more reflection magic on the side of k6.

[mstoykov]

My tangent about "the thing" instead of context was mostly to abstract the way the specific, as it doesn't matter really, even if just giving context.Context everywhere is the easier way.

Other than that, in general, I'd personally prefer a solution which makes writing Go modules as simple as possible, even if that means more reflection magic on the side of k6.

does that mean that you want the stick with common.Bind and try to "fix" it ?

I am probably just going to showcase a PoC of the two(three) options from above with something like k6/metrics next week. It shouldn't take me more than half a day (tests won't work probably).

[na--]

does that mean that you want the stick with common.Bind and try to "fix" it ?

Maybe 🤷‍♂️ I don't have a strong opinion either way, but the current way to add Go modules is at least simple and works quite well. It has a few gotchas and warts, so if we can remove or minimize them, I won't have a lot of objections to it. Still, 👍 for a PoC of an alternative approach, it will be much easier to see the tradeoffs when we have a code diff to evaluate.

[mstoykov]

closed by #2108