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

Add an access-tracking cache to be used in rules #2887

Merged
merged 5 commits into from
Dec 2, 2022

Conversation

J-N-K
Copy link
Member

@J-N-K J-N-K commented Apr 1, 2022

Closes #2084
Closes #2881
Closes #2943
Depends on #2886

This implements option 1 that was discussed in #2084 (which seems to be what most comments prefer). It is remotely based on the code here: https://github.com/openhab/openhab-addons/blob/4f4dfcca20f39c2d8ed059b67e1dba09c9c83957/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/scope/SharedCache.java.

Two separate key-value-caches are implemented:

  1. privateCache: It is created per scriptIdentifier and can only be accessed from the same script. It is not cleared between two subsequent runs of a rule but removed when the script is unloaded. This can be used e.g. to store timers.

  2. sharedCache: It is shared between all scripts. Every access to a certain key in the cache is tracked. The key is removed from the map if all scripts that ever accessed (put or get) that key have been unloaded.

Since script unloading is tracked by the framework nothing special needs to be done in the script itself. That makes it usable also in UI rules, where scriptUnloaded is not available.

I am currently investigating if I can auto-cancel ScheduledFutures and Timer when a key is removed (i.e. the last script that used the key was unloaded). ScheduledFuture<?> and Timer objects are cancelled when the script is unloaded in a private cache or if the last accessor was removed in the shared cache. They are not cancelled on remove because I might be that the script wants to use it or take special actions before cancellation.

@jpg0: do you think the preset name cache should be changed, so that we do not run into conflicts? I changed the case of the sharedcache to be more consistent with the camel-case we use in general. As an alternative, I could implement sharedcache as an alias to sharedCache and log a warning if that is used instead of the new name. The old name could then be removed in a later version.

Signed-off-by: Jan N. Klug github@klug.nrw

@J-N-K J-N-K requested a review from a team as a code owner April 1, 2022 14:25
@wborn wborn added the enhancement An enhancement or new feature of the Core label Apr 1, 2022
@jpg0
Copy link
Contributor

jpg0 commented Apr 2, 2022

@J-N-K tbh I have not been very careful with naming in the past because:

  • I personally always explicitly import everything as I've had huge issues in the past with these implicit things clashing
  • There has not been much care taken here so far in naming - for example event is surely one of the most used variable names possible!
  • Any add-on can just insert it's own new preset with whatever name it chooses at whatever time it chooses and cause clashes

As you are probably aware I have been against auto-importing these symbols due to these issues - however as both (auto-import and common names) things have already happened, my view now is that to remediate the problem we now should try to be clear to the user when a clash actually occurs (most likely by hooking into the JS engine) if it becomes a problem. (This hasn't been done.)

As for the immediate concern, I appreciate the concern of clashing with existing scripts but also fear needing to come up with more and more obscure preset names as we introduce additional ones. Not sure where we should draw the line though.

@J-N-K J-N-K changed the title Add an access-tracing cache to be used in rules Add an access-tracking cache to be used in rules Apr 3, 2022
@J-N-K
Copy link
Member Author

J-N-K commented Nov 6, 2022

@openhab/core-maintainers As this is a much requested feature it would be great if someone could find the time to review. About half of the changed lines are tests.

@openhab-bot
Copy link
Collaborator

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

https://community.openhab.org/t/motion-detected-light-on-turn-off-after-3-minutes-but-only-between-8pm-5am/141212/31

@jimtng
Copy link
Contributor

jimtng commented Nov 27, 2022

Could this make it into 3.4?

@florian-h05
Copy link
Contributor

This is also on our "wishlist" at JS Scripting, as well as it is closing 3 issues at once.

@openhab/core-maintainers Would be great if it could make it into 3.4.0.

@rkoshak
Copy link

rkoshak commented Nov 28, 2022

I'll chime in. From the end user's perspective, this will be very important to universally support many use cases that are only possible through sharing variables between script actions and conditions or preserving variables between runs of a single script action/condition.

I've been anticipating this PR's merge for some time. It would be awesome if we could get it into 3.4 release.

@jimtng
Copy link
Contributor

jimtng commented Nov 28, 2022

Who do we need to ping? Perhaps this is overlooked?

@florian-h05
Copy link
Contributor

I already pinged the core maintainers, if we want to, we can also explicitely ping kaikreuzer, cweitkamp or wborn.

@wborn
Copy link
Member

wborn commented Nov 29, 2022

Yes it will be a nice addition and it's not that much code too. 🙂

sharedCache: It is shared between all scripts.

I think it is truly shared between all scripts so you can also use it between different scripting engines, right? Might be handy if you decide to migrate from one engine to another one day.

Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Jan N. Klug <github@klug.nrw>
Signed-off-by: Jan N. Klug <github@klug.nrw>
@J-N-K J-N-K force-pushed the feature-scriptcache branch from 2595b79 to d520ca2 Compare November 30, 2022 20:01
@J-N-K
Copy link
Member Author

J-N-K commented Nov 30, 2022

It seems that there is an issue with using this new feature in JSScripting as well as DSL rules. @florian-h05, do you know how to import additional presets? According to the JSR223 documentation this should work via scriptExtension.importPreset but scriptExtension (or its alias se) seems to be unavailable in JSScripting. In Groovy the following works:

import org.openhab.core.automation.*
import org.openhab.core.automation.module.script.rulesupport.shared.simple.*
import org.openhab.core.config.core.Configuration

scriptExtension.importPreset("RuleSupport")
scriptExtension.importPreset("RuleSimple")
scriptExtension.importPreset("cache")

def sRule = new SimpleRule() {
    Object execute(Action module, Map<String, ?> inputs) {
	    sharedCache.put("x", "y")
    }
}
sRule.setTriggers([
    TriggerBuilder.create()
        .withId("aTimerTrigger")
        .withTypeUID("timer.GenericCronTrigger")
        .withConfiguration(new Configuration([cronExpression: "0 * * * * ?"]))
        .build()
    ])

automationManager.addRule(sRule)

@jpg0
Copy link
Contributor

jpg0 commented Nov 30, 2022

@J-N-K you can reference it like this:

const { cache } = require('@runtime/Defaults');

(assuming that it's in the default set of presets)

@florian-h05
Copy link
Contributor

florian-h05 commented Nov 30, 2022

@J-N-K
AFAIK, you are right that scriptExtension is not available, but reading throught https://github.com/openhab/openhab-addons/blob/622654ff1d020454c60980ccfc500905f7f2c068/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/ScriptExtensionModuleProvider.java, I think it should be possible to import the cache using require('@runtime/cache') (as it is not in the default preset, but in the cache presets).

const { sharedCache, privateCache } = require('@runtime/cache');

@J-N-K
Copy link
Member Author

J-N-K commented Dec 1, 2022

Thanks, I can confirm that

var { sharedCache } = require("@runtime/cache")

console.log(sharedCache.get("x"))

successfully logs y which is the value set in the Groovy script. So it works across languages, just DSL needs to be figured out. I'm currently not sure how to implement the privateCache, the sharedCache seems to be possible.

@J-N-K
Copy link
Member Author

J-N-K commented Dec 1, 2022

I also found a way to make the cache available in DSL. It feels a bit like an ugly hack, so I would prefer to do that in another PR, as it‘ll most likely be a longer review process.

@florian-h05
Copy link
Contributor

So this is finished and ready for review now?

@J-N-K
Copy link
Member Author

J-N-K commented Dec 1, 2022

Yes, it‘s also rebased and confirmed working with latest changes to automation.

…src/main/java/org/openhab/core/automation/module/script/rulesupport/shared/ValueCache.java
Copy link
Member

@wborn wborn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! 👍 Let's do some testing with a bigger audience. 🚀
Can you also create a PR to add some docs for this?

@wborn wborn merged commit 028724a into openhab:main Dec 2, 2022
@wborn wborn added this to the 3.4 milestone Dec 2, 2022
@wborn
Copy link
Member

wborn commented Dec 2, 2022

Is the plan now also to remove the existing SharedCache from the jsscripting add-on to prevent "user cache confusion"?

@florian-h05
Copy link
Contributor

Is the plan now also to remove the existing SharedCache from the jsscripting add-on to prevent "user cache confusion"?

Yes, but the coexistence should not lead to real user confusion, as the SharedCache is not documented directly and only exposed through the helper library.
I just need to figure out how to handle both the shared and the private cache in the JavaScript library whilst not breaking existing installations.
I‘ll think about this and probably ping a few people to get their opinion.

ccutrer added a commit to ccutrer/openhab-jrubyscripting that referenced this pull request Dec 2, 2022
ccutrer added a commit to ccutrer/openhab-jrubyscripting that referenced this pull request Dec 2, 2022
ccutrer added a commit to ccutrer/openhab-jrubyscripting that referenced this pull request Dec 2, 2022
ccutrer added a commit to ccutrer/openhab-jrubyscripting that referenced this pull request Dec 2, 2022
@J-N-K J-N-K deleted the feature-scriptcache branch December 2, 2022 17:07
ccutrer added a commit to ccutrer/openhab-jrubyscripting that referenced this pull request Dec 2, 2022
ccutrer added a commit to ccutrer/openhab-jrubyscripting that referenced this pull request Dec 2, 2022
ccutrer added a commit to ccutrer/openhab-jrubyscripting that referenced this pull request Dec 3, 2022
ccutrer added a commit to ccutrer/openhab-jrubyscripting that referenced this pull request Dec 3, 2022
@florian-h05
Copy link
Contributor

@J-N-K Does the cancellation of timers work for you in JS Scripting?

I am using the following script, and I can edit it and save it, openHAB reloads the script, but the timer runs (you need to have a look at the log timestamps, there should run no timer 15 seconds after the first reload of the script):

const { privateCache } = require('@runtime/cache');
const { actions, time } = require('openhab');

console.log(privateCache.get('timer'));

const timer = actions.ScriptExecution.createTimer(time.toZDT().plusSeconds(15), () => {
  console.warn('Timer ran.');
})

privateCache.put('timer', timer);

Log:

19:35:58.228 [INFO ] [port.loader.AbstractScriptFileWatcher] - Loading script '/etc/openhab/automation/js/test.js'
19:35:59.221 [INFO ] [penhab.automation.script.file.test.js] - null
19:36:06.185 [INFO ] [port.loader.AbstractScriptFileWatcher] - Loading script '/etc/openhab/automation/js/test.js'
19:36:07.172 [INFO ] [penhab.automation.script.file.test.js] - null
19:36:14.223 [WARN ] [penhab.automation.script.file.test.js] - Timer ran.
19:36:22.172 [WARN ] [penhab.automation.script.file.test.js] - Timer ran.

@J-N-K
Copy link
Member Author

J-N-K commented Dec 8, 2022

Can't work, because the import is wrong.

florian-h05 added a commit to florian-h05/openhab-addons that referenced this pull request Dec 11, 2022
Upgrades the included openhab-js version to 3.1.0, which uses the new
caches from core (introduced in
openhab/openhab-core#2887) and provides many
doc improvements.

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
florian-h05 added a commit to florian-h05/openhab-addons that referenced this pull request Dec 11, 2022
Removes the SharedCache from the addon because this functionality is
now provided by core (see
openhab/openhab-core#2887).

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
wborn pushed a commit to openhab/openhab-addons that referenced this pull request Dec 11, 2022
Upgrades the included openhab-js version to 3.1.0, which uses the new
caches from core (introduced in
openhab/openhab-core#2887) and provides many
doc improvements.

Removes the SharedCache from the addon because this functionality is
now provided by core (see
openhab/openhab-core#2887).

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
morph166955 pushed a commit to morph166955/openhab-addons that referenced this pull request Dec 18, 2022
Upgrades the included openhab-js version to 3.1.0, which uses the new
caches from core (introduced in
openhab/openhab-core#2887) and provides many
doc improvements.

Removes the SharedCache from the addon because this functionality is
now provided by core (see
openhab/openhab-core#2887).

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Ben Rosenblum <rosenblumb@gmail.com>
andrasU pushed a commit to andrasU/openhab-addons that referenced this pull request Dec 24, 2022
Upgrades the included openhab-js version to 3.1.0, which uses the new
caches from core (introduced in
openhab/openhab-core#2887) and provides many
doc improvements.

Removes the SharedCache from the addon because this functionality is
now provided by core (see
openhab/openhab-core#2887).

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Andras Uhrin <andras.uhrin@gmail.com>
borazslo pushed a commit to borazslo/openhab-mideaac-addon that referenced this pull request Jan 8, 2023
Upgrades the included openhab-js version to 3.1.0, which uses the new
caches from core (introduced in
openhab/openhab-core#2887) and provides many
doc improvements.

Removes the SharedCache from the addon because this functionality is
now provided by core (see
openhab/openhab-core#2887).

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
psmedley pushed a commit to psmedley/openhab-addons that referenced this pull request Feb 23, 2023
Upgrades the included openhab-js version to 3.1.0, which uses the new
caches from core (introduced in
openhab/openhab-core#2887) and provides many
doc improvements.

Removes the SharedCache from the addon because this functionality is
now provided by core (see
openhab/openhab-core#2887).

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
nemerdaud pushed a commit to nemerdaud/openhab-addons that referenced this pull request Feb 28, 2023
Upgrades the included openhab-js version to 3.1.0, which uses the new
caches from core (introduced in
openhab/openhab-core#2887) and provides many
doc improvements.

Removes the SharedCache from the addon because this functionality is
now provided by core (see
openhab/openhab-core#2887).

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 12, 2023
Signed-off-by: Jan N. Klug <github@klug.nrw>
GitOrigin-RevId: 028724a
andrasU pushed a commit to andrasU/openhab-addons that referenced this pull request Jan 6, 2024
Upgrades the included openhab-js version to 3.1.0, which uses the new
caches from core (introduced in
openhab/openhab-core#2887) and provides many
doc improvements.

Removes the SharedCache from the addon because this functionality is
now provided by core (see
openhab/openhab-core#2887).

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Andras Uhrin <andras.uhrin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automation enhancement An enhancement or new feature of the Core
Projects
None yet
7 participants