[jsscripting] Reimplement timer polyfills to conform standard JS

[florian-h05]

Fixes #13594.

Description

This PR reimplements the polyfills for the standard JS timers (setTimeout and setInterval) in the Java layer of the binding to conform standard JS.

This brings several improvements:

• Usage of TimerImpl from core is no longer required; instead the scheduler is accessed directly
• Mapping of ScheduledFutures to unique Integers to conform standard JS that returns a timeoutId or intervalId when the according funtion is run. FYI: Standard JS shares the same pool of integers for both, the binding does therefore the same.
• Scheduled jobs created by the polyfills are named by using the loggername, the type (timeout or interval) and the id (an Integer).
• All scheduled jobs created by the polyfills are automatically cancelled when the rule is unloaded to avoid having unmanagable scheduled jobs running endless (until restart of openHAB).

Breaking Changes

• The polyfills do not return an openHAB Timer anymore, instead they return an interger id as standard JS does.
• Other ways of scheduling timers (createTimer, createTimerWithArgument) are not recommended to be used anymore as they are not threadsafe. But it is still possible to access them via the ScriptExecution actions from core.
• I propose to log a warning when someone tries to access those timer creation methods via the ScriptExecution action from the openhab-js library.

Therefore, the polyfills for the standard JS timer methods should be the only way taken for creating timers in JS Scripting!

Testing

const { rules, triggers } = require('openhab');

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

rules.JSRule({
  name: 'Test Rule',
  triggers: triggers.ItemCommandTrigger('test_switch'),
  execute: (event) => {
    console.info('Rule 2 ran');
    // console.loggerName = 'org.openhab.automation.js.1123';
    let myVar = 'Hello world';

    // Create timers for the same timeout to ensure that timers/scheduled jobs are threadsafe
    for (let i = 0; i < 15; i++) {
      setTimeout(() => {
        console.info('setTimeout ' + i + ' completed successfully with message: ' + myVar);
        Thread.sleep(500);
      }, 2000, myVar);
    }

    let id;
    if (true) {
      // Create interval to verify functionality of setInterval
      id = setInterval(() => {
        console.info('setInterval completed successfully');
      }, 100, myVar, myVar);
    }

    if (true) {
      // Make a scheduled job fail to check whether naming jobs works
      setTimeout(() => {
        console.info(uninizializedVar);
      }, 1000);
    }

    if (true) {
      // Cancel interval with a timeout by using the intervalId
      setTimeout(() => {
        console.info('Attempting to clear setInterval...');
        clearTimeout(id);
      }, 5000, myVar, myVar);
    }

    myVar = 'Hello mutation';
  }
});

[florian-h05]

@jpg0 @digitaldan
This is my reimplementation of the timer polyfills.

[florian-h05]

To Do after Merge

openhab-js PR is now open: openhab/openhab-js#169.

Update the docs at openhab-js repo:

• Mention that the polyfills now behave like standard JS
• Remove the openHAB Timers from the docs
• Warn that changing the logger name can lead to messages not displayed in the logs due to missing logger configuration
• Remove the docs for the raw Java timer creation methods

Update the openhab-js codebase:

• Warn from access to the raw Java timer creation methods

• Add a breaking change warning to https://github.com/openhab/openhab-distro/blob/main/distributions/openhab/src/main/resources/bin/update.lst. PR is Add breaking change warning for JS Scripting openhab-distro#1420.

[jpg0]

@florian-h05 I would suggest that the better way to inject the ThreadsafeTimers object is the same way that is used for SharedCache, and everything in https://github.com/florian-h05/openhab-addons/tree/jsscripting-reimplement-timers/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/scope. By default these are injected at module paths, but you can simply remap them in the globals JS script. They have the ability to be unloaded automatically. You may need to pass the lock object differently, I have not looked into that. I do believe that it's worth it though, requiring custom JS when using setTimeout isn't good.

Oh and one other thing - ThreadsafeTimers is not thread safe itself (by concurrent accessors). I'm guessing that this is probably ok, but wanted to make sure you are expecting this.

[florian-h05]

@jpg0

I would suggest that the better way to inject the ThreadsafeTimers object is the same way that is used for SharedCache

Is it still possible then to have its own instance for each script engine?
Looking at the sharedcache, which is shared across all script engines.

Oh and one other thing - ThreadsafeTimers is not thread safe itself (by concurrent accessors). I'm guessing that this is probably ok, but wanted to make sure you are expecting this.

I’m not sure what you mean by that. Could you help me a little bit?

[jpg0]

Is it still possible then to have its own instance for each script engine?

Yes, this is the same as per-script. You can use scriptIdentifier to determine which script it belongs to.

I’m not sure what you mean by that. Could you help me a little bit?

I mean that if two or more threads concurrently access an instance of ThreadsafeTimers, then it may fail. If access is always done by a single thread it should be fine.

[florian-h05]

@jpg0
I managed to automatically cancel all scheduled jobs when the engine is unloaded without injecting ThreadsafeTimers the same way as SharedCache. In my opinion, choosing the ScriptExtension way of injection is much more complicated (and requires more code) and I also need to manage the ThreadsafeTimer instances using the scriptIdentifier.

I also synchronized another entrypoint to the context using the lock object; this entrypoint was accessed when the script file was reloaded.

I mean that if two or more threads concurrently access an instance of ThreadsafeTimers, then it may fail.

Got it, thanks. Until now, I've never seen a case where this happened.

[jpg0]

The problem with the current mechanism (rather than how SharedCache is done), is that each new piece of functionality that we need added to the environment is a custom job of explicitly modifying the creation of each environment. This is exactly the problem that the alternative mechanism solves - it is a framework for allowing new code to add new things to the environment without modifying the environment creation itself.

[jpg0]

Regarding thread safely: you are very unlikely to see it happen because it requires precise timing. I do believe that it's unsafe though, especially as recurring jobs are rescheduled on new threads. What it means is that very occasionally something will fail with a ConcurrentModificationException.

It's not hard to protect against however, I'm happy to add some suggested changes to fix it.

[florian-h05]

It's not hard to protect against however, I'm happy to add some suggested changes to fix it.

Thanks for the help. I'll wait with pushing new commits until you are finished.

[florian-h05]

@jpg0
I am running from one problem into the other regarding a implementation using the ScriptExtensionProvider, I won't try it this way anymore.

[florian-h05]

@jpg0

The problem with the current mechanism (rather than how SharedCache is done), is that each new piece of functionality that we need added to the environment is a custom job of explicitly modifying the creation of each environment.

I was thinking a bit about this fact, and I see your point but the timer polyfills are core functionality that is also provided by NodeJS, while the SharedCache is an openHAB specific extension to the JS runtime.

Given that, I would regard the current implementation as okay.

[jpg0]

You're right about the scope of these things, so it's not really clear that it belongs in the same place as SharedCache. It does still feel nasty to put ThreadsafeTimers directly into context creation though. How about just adding one layer of abstraction? Create a JSRuntimeFeatures (or whatever) class, which provides a map of things to be put into context creation. This class can just collect them from a list of other classes - currently just ThreadsafeTimers. This means that when we add more JS features it's clear where to add them and it will only be in a single place.

[jpg0]

Other than the minor comment above, it looks good. Happy with the scheduler change too.

As before, you'll have to get someone else such as @digitaldan to actually approve & merge as I don't have permission.

[florian-h05]

@openhab/add-ons-maintainers Can you merge?

Please see the last comment of jpg0 for review.

[jlaur]

First, thanks for the contribution! And kudos also to @jpg0 for the review and guidance. I'm not confident in this area, but nevertheless provided a few minor comments for the changed code.

The PR is tagged as a breaking change, how can we ensure that users are warned about this, and which changes will they need to do? Perhaps @kaikreuzer could also have a quick look since also involved in the previous iteration.

[florian-h05]

@jlaur
I addressed your review.

The PR is tagged as a breaking change,

Yes, I've added this tag as the change of the implementation of the polyfills for setTimeout and setInterval could possibly break existing installations. But this is only the case if the user's scripts expect that setTimeout and setInterval return an openHAB Timer class (see https://github.com/openhab/openhab-core/blob/18d063e272de14f4657ae925270f0d524f056fa1/bundles/org.openhab.core.model.script/src/org/openhab/core/model/script/internal/actions/TimerImpl.java#L31); this behaviour is not the standard JS behaviour.

how can we ensure that users are warned about this,

By adding a breaking change note to the release notes and adding an alert to https://github.com/openhab/openhab-distro/blob/main/distributions/openhab/src/main/resources/bin/update.lst.
PR is openhab/openhab-distro#1420.

how can we ensure that users are warned about this,

By adding a breaking change note to the release notes and adding an alert to https://github.com/openhab/openhab-distro/blob/main/distributions/openhab/src/main/resources/bin/update.lst.

and which changes will they need to do?

For the rare case that a user relies on the polyfills returning an openHAB Timer, he has to update his code to e.g. cancel the timer by running cancelTimeout or cancelInterval on the returned numeric id. To say it in some other way, the user has to adjust his usage of setTimeout and setInterval so that his code would run on NodeJS or in the browser.

Perhaps @kaikreuzer could also have a quick look since also involved in the previous iteration.

If you like to @jlaur, no problem. I'm just not sure if that's required since Kai onyl left a few short comments (the same sort that your comments are) and merged the PR, the changes itself were also guided and reviewed by jpg0.

[jlaur]

If you like to @jlaur, no problem. I'm just not sure if that's required since Kai onyl left a few short comments (the same sort that your comments are) and merged the PR, the changes itself were also guided and reviewed by jpg0.

Thank you for the detailed explanations. Mostly I was hoping to get a second opinion about this small breaking change, which to me seems like is for the better as otherwise we would block ourselves from moving forward, and it would only get worse.

Let's not rush it too much, but still aim for the upcoming milestone. If no one else will provide feedback today or tomorrow, I will merge.

[jlaur]

I propose to log a warning when someone tries to access those timer creation methods via the ScriptExecution action from the openhab-js library.

@florian-h05 - I assume this would be a PR to be merged into openhab-js after this PR is merged?

[florian-h05]

this small breaking change, which to me seems like is for the better as otherwise we would block ourselves from moving forward, and it would only get worse.

FYI also see #13594 (comment) for what @digitaldan said about reimplementing the polyfills.

Let's not rush it too much, but still aim for the upcoming milestone. If no one else will provide feedback today or tomorrow, I will merge.

👍

I assume this would be a PR to be merged into openhab-js after this PR is merged?

Yes, exactly. I'll prepare a PR in openhab-js to complete the tasks I mentioned at the beginning of this thead. The new release also needs to go into the milestone, so I'll ping you @jlaur for merging. (Technically, it does not need to, but I'd like to have up-to-date docs and the warning.)

[kaikreuzer]

If you like to @jlaur, no problem. I'm just not sure if that's required since Kai onyl left a few short comments

I agree, no need for an explicit approval by me. So @jlaur, if you are happy with the changes, please merge!

[jlaur]

LGTM

[florian-h05]

@jlaur
openhab-js PR is now open, see openhab/openhab-js#169.

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/openhab-3-4-milestone-discussion/138093/91