Recycle Otto VMs

[raidancampbell]

I think I've figured out a safe-ish way to recycle an otto VM for javascript execution. By adding an *otto.Otto to the Context struct and using a subcontext for ExecRuleAction's goroutine and in the javascript ProcessEvent callable we can prevent concurrent access.

In theory, there's a bug where altered state can be accessed between usages. In practice I haven't observed this causing a problem but I'm hesitant to consider it bug-free.

I think it should be possible to guarantee stateless executions with a few changes:

• javascript code blocks are turned into functions. They receive the env and bindings maps
• through some clever eval loop, hopefully there can be some boilerplate at the beginning of this function to expose the bindings like they are today.

If this works, it should guarantee no leakage between VM usages, as any code blocks live in their own function and state changes would live on that function's stack and be thrown away after the function is invoked. Users wouldn't have to make any changes as these changes would live entirely within javascript.go

Let me know if you have some thoughts on this. I did look into goja, and found otto to still be faster and less memory intensive for no-op benchmarks:

• otto: 97669 124328 ns/op 154959 B/op 1507 allocs/op
• goja: 49501 241823 ns/op 266827 B/op 3199 allocs/op

[jsccast]

Just some quick thoughts for now:

1. We'd want to make a deep copy of those bindings so that Javascript couldn't mess with anything outside of its sandbox. Maybe we're already doing that? (If a deep copy failed due to (say) encountering some native, uncopyable Go thing, then perhaps the code really should return an error.)
2. I think what'd we really want is a way to seal a runtime. otto hasn't support that capability in the past AKIAK.
3. Without runtime-level sealing/freezing/whatever, seems like mean Javascript can always cause trouble. Here's an example. Or even just this?
4. Since the safety is possibly an open question, could we introduce a configuration switch (like "unsafeJavascript"!) to allow the brave author reuse Javascript runtimes when desired?

[raidancampbell]

I'll see how much performance I can get without tackling this problem: I'm lazy, and you've brought up several good points.

1. good point, I hadn't considered that
2. I was hoping a function would buy enough isolation, but it's clear that it doesn't
3. here's where I think we have different use cases. In my use case all javascript is trusted, so I fail to consider untrusted code in my solution
4. I think that's a good solution. whether the mechanism is at rule-level or rulio-level is open, but I'll cross that bridge if I get to it.

[jsccast]

Re trusted code: Yeah, that notion suggests a better switch name: TrustedJavascript? Or perhaps more directly just ReuseJavascriptRuntimes?

[raidancampbell]

As much as I like TrustedJavascript, I think ReuseJavascriptRuntimes is a better choice here. TrustedJavascript implies a set of options, and would cause confusion with the existing JavascriptTimeouts switch.

[raidancampbell]

I've been simmering on this for a couple weeks and have two different options. The functionality for either would live behind a default-off toggle as discussed:

• VM per bindings
  • the fork happens in location.WorkWalk's execRuleAction goroutine (events.go)
  • VM is held in the context: subcontexts do not inherit the VM.
  • favors deeper rules
  • fairly safe, as the lifespan of the VM is short. Additional guarantees can come from the below scope changes
• VM pool via sync.Pool
  • needs code blocks to be wrapped into (function(){%s return %s})() functions to lower risk of scope leakage
    • needs AST parsing. the last line needs to be a return statement for this to work correctly. otto may no longer work here, I see no easy way of achieving this with its parsing (likewise for goja). If we assume javascript code is only newline delimited (e.g. the last line of code isn't x = 5; x = x + 1), then the problem is solved by simply splitting on newlines and putting return before the last. This would be a breaking change (granted its held behind a toggle) and I'm not sure I like it.
    • moving everything to let instead of var and just using {} scopes should fix this
      • let is ECMAScript 6, and both otto and goja only support up to 5
      • babel may fix this?
      • scoping in {} would be the only rulio change here. otherwise it would be rules changes to have a let-like operator implemented and use it in all rules. very ugly.
  • still has risk of global state change
  • requires some refactoring between CompileJavascript and RunJavascript but shouldn't be a big problem
  • most efficient for all shallow and deep rules

I really want the sync.Pool implementation to work. What I'm looking for here is whether the return hack is acceptable or if you have some additional guidance/thoughts on the matter. I've got some very work-in-progress code for both options if you want to see something concrete.

[raidancampbell]

dop251/goja#291 has been raised to see if goja has any input on preventing state change in VM reuse. It'd be a win-win if there's an easy solution in goja, otto is looking more and more like it's been abandoned

[jsccast]

I'm wondering if your switch for ReuseJavascriptRuntimes could require/assume that all Javascript already have the necessary return. In other words, that switch would really mean two things: A Javascript runtime could be reused and you have to write slightly different Javascript, too. That'd avoid the unpalatable Javascript AST parsing while still being backwards-compatible (when the switch isn't thrown).

[raidancampbell]

I like it. That would lend to another name change to something like ScopedJavascript and need documentation in the switch and the code block docs. I'll get some code cleaned up for a PR next week

[raidancampbell]

hmmm. I've written maybe a dozen lines of javascript before, but there is another headache that's arising: variable declaration like x = 1 is in the global state and accessible outside of functions. so running the following code in a regular browser javascript console returns 1 instead of an error on undefined variable:

(function(){x=1})()
x

Declaring with var solves it.

It falls into the same category of "when this switch is thrown, assumptions change like needing a return statement", but this is another batch of required rule changes before anyone could enable the switch. Better naming and documentation should still get the point across, but this looks like a footgun to me.

I'm still planning on raising a PR, but enabling it is a bit scarier than I'd hoped.

[raidancampbell]

Closing due to inactivity (and I'm no longer involved in any project that uses this)