Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Blockly OH blocks compatible with the JSScripting add-on #1170

Closed
wants to merge 1 commit into from

Conversation

ghys
Copy link
Member

@ghys ghys commented Sep 29, 2021

This prepends code that makes the ctx, itemRegistry & events
objects accessible when the scripts are run with GraalVM, as they
are when run with Nashorn.

Closes #1169.

Signed-off-by: Yannick Schaus github@schaus.net

This prepends code that makes the `ctx`, `itemRegistry` & `events`
objects accessible when the scripts are run with GraalVM, as they
are when run with Nashorn.

Closes openhab#1169.

Signed-off-by: Yannick Schaus <github@schaus.net>
@ghys ghys requested a review from a team as a code owner September 29, 2021 09:52
@relativeci
Copy link

relativeci bot commented Sep 29, 2021

Job #211: Bundle Size — 10.58MB (-0.67%).

e6ae713 vs a51895b

Changed metrics (5/8)
Metric Current Baseline
Initial JS 1.63MB(-3.88%) 1.7MB
Initial CSS 602.22KB(-0.56%) 605.64KB
Cache Invalidation 23.61% 16.11%
Modules 1499(-0.4%) 1505
Packages 129(-0.77%) 130
Changed assets by type (2/7)
            Current     Baseline
CSS 845.32KB (-0.4%) 848.74KB
JS 8.31MB (-0.81%) 8.38MB

View Job #211 report on app.relative-ci.com

@rkoshak
Copy link

rkoshak commented Nov 30, 2021

While experimenting with the JSScripting add-on I discovered that java.lang.Thread.sleep() is not allowed. That I believe will have some impact on this PR. We might need to remove the sleep block for JSScripting or at least radically change it. I tried to use a Promise and await and it did solve the exception that gets thrown but it never returns from the await.

@stefan-hoehn
Copy link
Contributor

... We might need to remove the sleep block for JSScripting or at least radically change it...

I doubt we can do this as this will most likely break hundreds of people's rules because I am sure that thread-sleep is used very often.

@rkoshak
Copy link

rkoshak commented Feb 21, 2022

I doubt we can do this as this will most likely break hundreds of people's rules because I am sure that thread-sleep is used very often.

A busy wait loop might be a work around. No one like it but I've used it in the past quite successfully in JS Scripting, mostly for unit tests.

The problem is two fold:

  1. Nashorn is going to go away. Based on some recent issues and PRs I've seen, maybe even sooner than I thought. So Blockly must be changed to use JS Scripting.
  2. GraalVM doesn't allow messing with threads in any way. Each rule gets one thread to run in and it can't modify it in any way. Therefore trying to sleep results in an exception.

There is no work around found so far (the usual JavaScript promise and await doesn't work because there's no event loop).

So I would recommend:

  1. implement a busy wait to replace calls to Thread:sleep
  2. mark the block as deprecated to discourage it's use
  3. at some point (maybe a year or so, maybe wait for OH 4) remove the block entirely

Of course, if a work around is found to replace it in the mean time replace the busy wait with that work around.

@florian-h05
Copy link
Contributor

Bringing some activity back to this PR discussion …

While experimenting with the JSScripting add-on I discovered that java.lang.Thread.sleep() is not allowed.

That is not true.
A few weeks ago when working at the multi-thread access fix for setTimeout and setInterval (see openhab/openhab-addons#13582), I was successfully using Thread.sleep. I have just rechecked, and it is working with JS Scripting:

const Thread = Java.type('java.lang.Thread');

console.info("Hello");
Thread.sleep(5000);
console.info("world");

@rkoshak
Copy link

rkoshak commented Nov 14, 2022

That is not true.

Not true any more. It was a problem when I wrote the above though. It would generate the "Multithreaded access no allowed for JS" exception. Quite some time ago it stopped happening. I can't remember when so I couldn't point at what changed to make it work.

Of course, if you use a sleep and a Timer and the Timer went off before the sleep was done, you'd be guaranteed to get that exception even now I think. I'm not sure if it'd still be a problem after your recent changes to make timers thread safe. I'm hoping the error won't occur and the timer will just have to wait.

@florian-h05
Copy link
Contributor

florian-h05 commented Nov 14, 2022

if you use a sleep and a Timer and the Timer went off before the sleep was done, you'd be guaranteed to get that exception even now I think. I'm not sure if it'd still be a problem after your recent changes to make timers thread safe.

Thread.sleep and timers that run off do not lead to that exception anymore (at the moment only timers created by setTimeout and setInerval are thread-safe, the PR for createTimer is awaiting a core PR). See my testscript at openhab/openhab-addons#13582.
When openhab/openhab-addons#13695 is merged, all timer creation methods that are not deprecated (createTimerWithArgument is deprecated) are thread-safe and can therefore be used together with Thread.sleep.
Thread.sleep in a script only makes the timer that runs off wait as long as Thread.sleep is sleeping (multi-thread access is synchronized on a lock object).

@stefan-hoehn
Copy link
Contributor

@florian-h05 Not to promise too much, but I could start testing all blocks based on JSScripting. Can you provide some documentation how I can try it with and then again without JSScripting? This could also help for providing documentation on it which I usually write in terms of blocklies.
It will definitely take quite some time as we have more than 50 blocks to be tested but at least I could start working on it.

@florian-h05
Copy link
Contributor

Can you provide some documentation how I can try it with and then again without JSScripting?

Do you mean how you can switch between JSScripting and Nashorn?
You have to change the script‘s MIME type, but I am not sure how that’s possible (maybe through the Rest API?).

For the usage of JS Scripting itself, the addon docs should help.

The big question here is: do we just want to make the blocks run on JS Scripting by re-adding access to the raw Java APIs (as done in this PR), or do we want to use the openHAB JavaScript library that comes with JS Scripting?
I expect the second option to be more work (but the first option can also be some work, as Nashorn and JS Scripting have their differences - I have to check the Blockly code to say something about this).
But I prefer the second option, as I think there are users which use Blockly to actually learn the JavaScript coding, and I don’t want them to „learn“ using the raw Java APIs because they are much more complicated than what the openhab-js library provides.

This could also help for providing documentation on it which I usually write in terms of blocklies.

JS Scripting docs?? Or what do you mean?

@ghys
Copy link
Member Author

ghys commented Nov 16, 2022

But I prefer the second option, as I think there are users which use Blockly to actually learn the JavaScript coding, and I don’t want them to „learn“ using the raw Java APIs because they are much more complicated than what the openhab-js library provides.

That would be best indeed.

The Blockly code generation should strive to present the best code possible to users, so that by switching from the blocks view to the code view, they can learn how they can perform the same task elegantly should they choose to leave Blockly behind and write their own code.

With that in mind, the implementations for adding or reading an item's metadata should be radically different when the Blockly code generation is supported by JSScripting (where it's just a openhab-js call) or by Nashorn (where we had to basically implement it by instantiating Java objects and so on).

I know this is a debate that we already had (have a fool-proof implementation regardless of how complex or convoluted it gets vs. elegant and straightforward code that could fail). I stick to my guns and think the latter is best.

The openhab-js library could provide that "fool-proof layer" so the generated code could remain simple (for instance, sending a command would accept any possible type and let the library deal with the necessary conversions as appropriate).

I wouldn't mind having 2 parallel implementations of the code generation for blocks, one would be for Nashorn, and another for JSScripting (which would make use of the openhab-js library). It's just a light refactoring with 2 files and some logic. We could (should?) even think about blocks being available or not based on the script engine.

The thing to keep in mind is that for this to work, we'd need stable APIs in the openhab-js library so that the blockly scripts don't break because the library was updated "out-of-band" with npm.

@florian-h05
Copy link
Contributor

I wouldn't mind having 2 parallel implementations of the code generation for blocks, one would be for Nashorn, and another for JSScripting (which would make use of the openhab-js library).

I don’t know how Blockly exactly works, but that brings me to the another question:
When you’ve created a script with blocky, and later reopen it: Does Blockly reload the the arrangement of blocks or are the blocks re-rendered by parsing the actual source code?
If the second is the case, we need to keep those 2 parallel implementations.
Otherwise, if it is possible to automatically migrate the Blockly scripts from Nashorn to JS Scripting, I wouldn’t support two implementations.
Depending on our decision, JS Scripting also needs to be installed by default.

We could (should?) even think about blocks being available or not based on the script engine.

If possible, I would prefer to keep all current blocks. But I think that we can extend the available blocks because openHAB-JS covers large parts of core functionality/APIs.

The thing to keep in mind is that for this to work, we'd need stable APIs in the openhab-js library so that the blockly scripts don't break because the library was updated "out-of-band" with npm.

Generally, we have stable APIs in openHAB-JS and avoid breaking changes, as they are always a problem because the users face them, not other devs.
There have been a few bugs in releases that lead to something not working, I’ve fixed them fast but the problem is getting them into openHAB. To avoid such problems in the future, we are working on heavily extending our unit test coverage.

florian-h05 added a commit to stefan-hoehn/openhab-webui that referenced this pull request Nov 27, 2022
Based on openhab#1170.

Also-by: Yannick Schaus <github@schaus.net>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@stefan-hoehn
Copy link
Contributor

@ghys @rkoshak Florian and I had a several hours conversation today and came up with a solution how to approach the migration to JSScripting and we will work on that in the weeks to come. We will keep you posted.

@stefan-hoehn stefan-hoehn mentioned this pull request Dec 19, 2022
5 tasks
florian-h05 added a commit to stefan-hoehn/openhab-webui that referenced this pull request Dec 19, 2022
Based on openhab#1170.

Also-by: Yannick Schaus <github@schaus.net>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
florian-h05 added a commit to stefan-hoehn/openhab-webui that referenced this pull request Dec 19, 2022
Based on openhab#1170.

Also-by: Yannick Schaus <github@schaus.net>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05
Copy link
Contributor

@ghys Let's close this PR and use #1597 to continue discussion/track the portation of Blockly.

@ghys ghys closed this Dec 20, 2022
florian-h05 added a commit to stefan-hoehn/openhab-webui that referenced this pull request Jan 8, 2023
Based on openhab#1170.

Also-by: Yannick Schaus <github@schaus.net>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
ghys pushed a commit to stefan-hoehn/openhab-webui that referenced this pull request Jan 22, 2023
Based on openhab#1170.

Also-by: Yannick Schaus <github@schaus.net>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove Blockly as an option when GraalVM JavaScript add-on is installed
4 participants