Migrate Blockly to JS Scripting (GraalJS)

[florian-h05]

Awaiting #1601.

The current situation

In #1170, a first solution how to make the Blockly blocks compatible with JS Scripting (GraalJS) was proposed, however this solution has two problems:

• it won't work anymore: GraalJS and Nashorn, which is currently used but will go away with openHAB 4, have seperate MIME types
• it doesn't present the "best" possible code to users

To port Blockly to JS Scripting, we therefore came to the conclusion (also in #1170), that Blockly should use the integrated openhab-js library instead of the raw Java APIs.

Our approach/solution

Blockly maintainer @stefan-hoehn and I (openhab-js and JS Scripting addon maintainer) joined forces and decided on the following approach:

• Support both Nashorn and GraalJS at the same time (= support code generation for both)
• GraalJS will be available by default on openHAB 4 and will own the default MIME type, so generate for GraalJS by default
• Existing Blockly scripts will get automatically converted when save is clicked
• If Blockly is on GraalJS, provide additional functionality, e.g. [Quantity] Initial contribution: Support UoM handling by wrapping QuantityType openhab-js#206
• If one wants to use Nashorn for Blockly, Add tags & script language to script settings #1601 can be used to set Nashorn as the engine

Concerns

• GraalJS is more strict on string concatenation, e.g. when adding a string to an Item‘s state: This is no problem, since openhab-js avoids returning Java objects where possible and uses JS natives, e.g. for Item state and Thing status we use string.
• Notably differences in the implementation of timers and (multi-)threading: Yes, they exist, but nearly all entry points are synchronized, means all timer creation APIs (setTimeout, setInterval, createTimer) create threadsafe timers and Thread.sleep also works.
• Cancelling a running timer or a running Thread.sleep can to a thread/sleep interrupted warning: This is by design, but it will never occur with Blockly or generally from a single script (multi-thread synchronization makes it impossible to call cancel on a currently executing timer). This can only happen when a currently executing timer created by script a is cancelled by script b (to make this possible, the timer must be shared across the scripts).
• The above described Problem can occur when using sharedScript. We should address the log issue.
• Running a rule from another rule can lead to multi-thread access requested but not allowed: This is still a problem, but it can be solved. I‘ll have a look at this.

To Dos

With the openHAB 4 release on the horizon, which removes Nashorn from core but readds it to the addons, I propose to change/extend that approach a little bit (under the assumption that GraalJS is installed by default, Nashorn not):

• Allow the user to easily change the script type in the UI (no YAML, no REST API call needed for the user): Add tags & script language to script settings #1601

Interesting links

• Shared and private cache for Blockly: [Blockly] Modify the Value Storage blocks to use the new sharedCache and privateCache #1577
• Addition of Nashorn to addons: Add JS Scripting (Nashorn) addon to addons openhab-addons#14005
• Latest JS Scripting (GraalJS) readme: https://github.com/openhab/openhab-js#readme
• Latest JSDoc for openhab-js (provides a more detailed doc of the API): https://openhab.github.io/openhab-js/

/cc @stefan-hoehn @ghys @rkoshak

[ghys]

(under the assumption that GraalJS is installed by default, Nashorn not)

I'm under the same assumption.

GraalJS and Nashorn, which is currently used but will go away with openHAB 4, have seperate MIME types

In openhab/openhab-core#2433 (comment) I argued that once GraalJS becomes the default JS engine, maybe by the time OH4 is released it should "own" the generic application/javascript media type (without parameters) and the Nashorn ScriptEngine add-on should use something like application/javascript;engine=Nashorn or application/javascript;version=ECMAScript-5.1.

The jury's out on whether it's a good idea, but in theory it could go smoother than we would normally expect, without much needing to alter scripts (Blockly or not) if the "global"/scope variables like ctx, itemRegistry or events are available. It's true that it wouldn't be the best code, because it would be oblivious to the openhab-js library existing, but it would still probably mostly work, or only with minor changes. One problem I had seen is that GraalVM is much more strict when you e.g. concatenate a state and a string as they have different types, so you have to make sure they both are vanilla JS strings. There are also notable differences in implementation relating to threading and timers AFAIK.
If the rules are indeed breaking in GraalVM then it's a simple matter of changing the type parameter in the affected module so that they switch back to Nashorn. This could also be the case for Blockly (in regular rules you can do that with the YAML view but there should also be an option for scripts where the type appears in the navigation bar but is not modifiable).

That way the Blockly script actions would keep the application/javascript type as they do now. Then they would just be a save/Ctrl-S away from being converted to the newer and better code, if they don't work as-is (what #1170 tried to do).
(note that the standard blocks in Blockly like loops or iterators very conservative on what JS version they target, so you'll find no newer features like arrow functions, let/const, template strings and the like in their code.)

[rkoshak]

One problem I had seen is that GraalVM is much more strict when you e.g. concatenate a state and a string as they have different types, so you have to make sure they both are vanilla JS strings.

When using openhab-js to get the Item, the Item.state will be a String. Only rawState will be the raw Java Object. So perhaps that specific problem would fall out if we move to use openhab-js (which I also strongly recommend because the amount of code Blockly will have to generate will be greatly reduced. And that code will be much easier to read and understand.

There are also notable differences in implementation relating to threading and timers AFAIK.

A lot of that has been worked out I believe. Thread.sleep is supported without throwing an error now, IIRC, and @florian-h05 did a lot of work to add locks around Timers so they cannot run at the same time and generate that multithreaded exception any more. You can still run into that exception if two rules try to call the same other rule at the same time, but within a given rule it's no longer a problem.

This could also be the case for Blockly (in regular rules you can do that with the YAML view but there should also be an option for scripts where the type appears in the navigation bar but is not modifiable).

I tried out OH 4 snapshot today and ran into this. We definitely need to be able to edit that. In fact, it'd be nice to edit other stuff about a Script too like tags, name, and description. But that's a discussion for another issue. ;-) Maybe adding a "Code" tab to see the YAML even for Scripts would be sufficient?

let/const, template strings and the like in their code.

let/const are still a problem in UI rules. The context gets reused so the second time the rule runs it'll complain that you can't redeclare a const or let. So this is a good thing.

[florian-h05]

in theory it could go smoother than we would normally expect, without much needing to alter scripts (Blockly or not) if the "global"/scope variables like ctx, itemRegistry or events are available.

If you want everything that was available in Nashorn in the global scope of GraalJS, it should be enough to add the following:

Object.assing(this, require('@runtime');

If the rules are indeed breaking in GraalVM then it's a simple matter of changing the type parameter in the affected module so that they switch back to Nashorn.

(Referencing my first answer …) But I am concerned that user written scripts run into muti-threading problems when they create timers from the not threadsafe APIs (ScriptExecution from core). So for user written scripts, the MIME type should be swapped to use Nashorn. We therefore need to allow the user to easily change the type of scripts in the UI (no YAML, no REST API calls).

One problem I had seen is that GraalVM is much more strict when you e.g. concatenate a state and a string as they have different types, so you have to make sure they both are vanilla JS strings.

That‘s because Graal behaves much more like Node than Nashorn does. As Rich already pointed out, openhab-js returns the state as string so concatenating Item state and string works fine. FYI: openhab-js tries to avoid to return any raw Java objects, instead e.g. for Item state and Thing status we use string. So that concern is solved.

There are also notable differences in implementation relating to threading and timers AFAIK.

Yes, Nashorn allows multi-thread access to the script context, Graal doesn‘t. Graal is this way to emulate the single-threaded concept of Node, but Graal is missing the event loop.
As Rich also pointed out, I did a lot of work to make JS Scripting threadsafe.
Our implementations of setTimeout and setInterval and our reimplementation of the createTimer method are threadsafe (synchronised using a ReentrantLock). The threadsafe version of createTimer is exported by openhab-js to actions.ScriptExecution.createTimer (internally, the library doesn‘t use the ScriptExecution actions from core, but a reimplementation in the addon that is injected into the runtime).

Thanks to the multi-thread access synchronization, it is also possible to use Thread.sleep without any problems.

Please note that when you cancel a timer that is currently executing, you‘ll get a WARNING that says something like Thread interrupted, or if you cancel a executing Thread.sleep something like sleep was interrupted (I currently can‘t remember the exact wording).
This is by design, because the cancelling of a timer is configured to interrupt if the timer is currently running. If we don‘t like that behaviour, we could possibly change it (required a one-liner in the addon and a few lines in core, but for core that change would need to be discussed).
IMO, that behaviour is fine as it is, but I‘d like to hear your opinions, especially from @rkoshak as well.

You can still run into that exception if two rules try to call the same other rule at the same time, but within a given rule it's no longer a problem.

I wan‘t aware of this, this needs a change in core but it shouldn‘t be too complicated. I‘ll talk about this with the core maintainers.

@ghys @rkoshak
I added a concerns section to the issue description, put all concerns there and explained a bit.

[ghys]

Thanks for the detailed & interesting explanations.
The main point I was raising above (even if going beyond Blockly) is that for "purity"'s sake, the generic application/javascript should be reserved for & assigned to the "primary" JS add-on, which from OH4 on would be GraalVM. By extension, Blockly script modules should be written for application/javascript by default.

So either we choose to keep the media/MIME types as they are now, or proceed with the above at the risk of potentially breaking existing scripts, which could be mitigated by:

1. either the user changing the type parameter manually (could also be done automatically in some cases - but I don't like it too much when things like this happen without the user's knowledge)
2. trying as much as possible to help make the Nashorn scripts work in GraalVM without modifications (as far as possible). The rest is for the user to fix if they didn't go with 1. For Blockly this would only involve opening & saving, in theory, which would rewrite the code with the JSScripting+library implementation.

[florian-h05]

The main point I was raising above (even if going beyond Blockly) is that for "purity"'s sake, the generic application/javascript should be reserved for & assigned to the "primary" JS add-on, which from OH4 on would be GraalVM. By extension, Blockly script modules should be written for application/javascript by default.

I totally agree. @rkoshak seems to also agree here, see openhab/openhab-addons#14005 (comment).

proceed with the above at the risk of potentially breaking existing scripts, which could be mitigated by:

I agree here as well as on your options described after. I'm not sure how 2 is meant, but IMO we shouldn't append code to Nashorn user scripts to make them work with Graal, but instead recommend to use Nashorn for them.

[ghys]

I'm not sure how 2 is meant, but IMO we shouldn't append code to Nashorn user scripts to make them work with Graal, but instead recommend to use Nashorn for them.

Sorry that was unclear indeed. I was thinking about in-place migration techniques from Nashorn to Graal for user scripts (as Blockly would be handled by a save). One of these could perhaps be to start adding Object.assing(this, require('@runtime'); first (the user, not automagically), figure out if things work again and iron out the issues, then switch to the new API (i.e. itemRegistry.getItem becomes items.getItem etc.).
It might be something that's documented in a OH3-OH4 migration guide, stressing that the change of engines involves some work and is not a drop-in replacement; the UI wouldn't have to support any of this though.

[rkoshak]

We therefore need to allow the user to easily change the type of scripts in the UI (no YAML, no REST API calls).

Or as part of the upgrade perhaps?

This is by design, because the cancelling of a timer is configured to interrupt if the timer is currently running. If we don‘t like that behaviour, we could possibly change it (required a one-liner in the addon and a few lines in core, but for core that change would need to be discussed).

At least in the case of a cancelled timer, which is a normal thing to do, it should be INFO, not WARN level I would think.

Maybe we don't have control over that? It's not unusual to have lots of Timers whose only purpose for existing is to be canceled. Put another way, the fact that it's cancelled is the "good" path and only if the Timer runs has something gone off. Getting a WARN level log on the good path is jarring and I know the first thing we'll see is "how do I fix this warning in my logs?" from users.

I wan‘t aware of this, this needs a change in core but it shouldn‘t be too complicated. I‘ll talk about this with the core maintainers.

👍

I haven't brought it up myself because I haven't yet decided if this is an antipattern or not. But it should probably be consistent and if we can fix this, we probably should.

It might be something that's documented in a OH3-OH4 migration guide, stressing that the change of engines involves some work and is not a drop-in replacement; the UI wouldn't have to support any of this though.

That seems reasonable to me. From the end user's perspective the blocks are the code. So we might need to hold their hand a little bit but it's reasonably to expect to need to do some manual steps to go between major release versions.

[florian-h05]

Or as part of the upgrade perhaps?

I am not sure how much "automatic" stuff we want and how complicated it is to automate such things. Before we implement that, we should implement the "manual" ways.

At least in the case of a cancelled timer, which is a normal thing to do, it should be INFO, not WARN level I would think.

I've just checked what GraalJS does when you cancel the timer from it's own callback function, I haven't got a warning :-)
FYI I used the following code snippet:

 var timer = actions.ScriptExecution.createTimer('timer1', time.toZDT().plusSeconds(10), () => {
  console.log('Timer is now running, attempting to cancel it...')
  timer.cancel()
  console.log('Timer has been cancelled.')
})

For the case that a timer is using Thread.sleep, you got a InterruptedException in the past, but I can't reproduce that anymore. It is logical that I cannot reproduce it because the synchronization mechanism doesn't allow timer a to cancel timer b while timer b is running (both created from the same script). If you however cancel a timer from another script (which is impossible with Blockly, because Blockly doesn't share it's timers), I can imagine getting a InterruptedException.

So it seems like we don't have to expect any InterruptedException warnings for the most use cases, and never for Blockly.

I've updated the issue description on top.

[rkoshak]

(which is impossible with Blockly, because Blockly doesn't share it's timers

With the sharedCache would that remain the case?

[florian-h05]

If Blockly uses the sharedCache, that wouldn’t remain the case.
Then it would be possible to cancel a currently executing timer from another cache.
But currently I am using the privateCache for Blockly timers.

[rkoshak]

I only bring that up because I think we will need to support sharedCache at some point so this cancelled timer log issue will need to be addressed eventually.

[stefan-hoehn]

If Blockly uses the sharedCache, that wouldn’t remain the case. Then it would be possible to cancel a currently executing timer from another cache. But currently I am using the privateCache for Blockly timers.

Does it make sense then to mention it later in the docs that it is not recommended to cancel a timer in a shared cache?

[rkoshak]

I still think it's OK to cancel it. Maybe just a warning that you'll see warnings in the logs but they are harmless.

[kaikreuzer]

(under the assumption that GraalJS is installed by default, Nashorn not)

I'm under the same assumption.

I am actually not - please see my comment on openhab/openhab-distro#1455.

[florian-h05]

Since the PR has been merged, we should start solving the remaining issues:

• Update docs to warn that a warning might get logged when cancelling a timer in the shared cache @rkoshak With the Blockly implementation of timers, it is not possible to store a timer in the shared cache and therefore this issue can never occur. We don‘t need to add such a note to the docs.
• Make runRule thread-safe: Running a rule from another rule can lead to multi-thread access requested but not allowed

[rkoshak]

With the Blockly implementation of timers, it is not possible to store a timer in the shared cache and therefore this issue can never occur. We don‘t need to add such a note to the docs.

Is this limited to Blockly or in general? My concern with that is one of the biggest reasons to share variables between rules is explicitly to share Timers (e.g. one rule that checks for existence, one that creates, one that cancels.

[florian-h05]

Is this limited to Blockly or in general? My concern with that is one of the biggest reasons to share variables between rules is explicitly to share Timers (e.g. one rule that checks for existence, one that creates, one that cancels.

This is no general limitation, and it is possible to support this as well from Blockly.
Currently, there are just no blocks that support storing timers in the shared cache.

[stefan-hoehn]

@rkoshak you challenged me ;-)

if (cache.private.exists('MyTimer') === false || cache.private.get('MyTimer').hasTerminated()) {
  cache.private.put('MyTimer', actions.ScriptExecution.createTimer('MyTimer', time.ZonedDateTime.now().plusSeconds(10), function () {
    cache.shared.put('SharedTimer', (cache.private.get('MyTimer')));
    cache.private.remove('MyTimer');
  }));
};