[jsscripting] Clarify that rules from one automation/js/.js file cannot run in parallel#19410
[jsscripting] Clarify that rules from one automation/js/.js file cannot run in parallel#19410dilyanpalauzov wants to merge 1 commit intoopenhab:mainfrom
Conversation
|
This change is incomplete. If I create at the same time a file automation/js/a.js console.info("START2 " + Java.type('java.lang.Thread').currentThread().getId())
Java.type('java.lang.Thread').sleep(30000)
console.info("END2")and automation/js/b.js console.info("START1 " + Java.type('java.lang.Thread').currentThread().getId())
Java.type('java.lang.Thread').sleep(30000)
console.info("END1")openhab.log contains: So all /js/*.js files are executed in a single thread, but if they create rules, the rules get their own thread. |
1510363 to
ca2d006
Compare
… their own threads, plural. (one thread per rule, or each rule uses a thread from a pool). |
ca2d006 to
d134386
Compare
No - each rule has a dedicated thread. |
I guess you mean each rule has a dedicated thread. |
rkoshak
left a comment
There was a problem hiding this comment.
Some suggestions on the additions.
|
|
||
| For [file based rules](#file-based-rules), scripts will be loaded from `automation/js` in the user configuration directory. | ||
| When many rules are installed from a single `.js` file in this directory, at most one of these rules runs at a time. | ||
| Irrespective of how JavaScript installs rules, each rule runs in a separate thread. |
There was a problem hiding this comment.
I recommend removing this line. All rules have their own thread and that's implemented and controlled by core. It's not JS specific.
Furthermore, it's not meaningful nor useful information for JS Scripting users. They can't do anything with this information. There is nothing a user can or should do with this information they are not already doing. It just raises questions.
| For [file based rules](#file-based-rules), scripts will be loaded from `automation/js` in the user configuration directory. | ||
| When many rules are installed from a single `.js` file in this directory, at most one of these rules runs at a time. | ||
| Irrespective of how JavaScript installs rules, each rule runs in a separate thread. | ||
| All `automation/js/.js` files are executed sequentially in a single thread. |
There was a problem hiding this comment.
I don't understand what this means. Are you referring to how the files themselves are loaded or restating the first sentence in another way? I assume the former. Maybe:
All
automation/js/*.jsfiles are loaded and executed sequentially and alphabetically in a single thread.
The "in a single thread" is kind of redundant, but I leave it up to you whether to keep it.
Conceptually, most end users are not really aware that these files are executed to create the rules (it should be obvious but for many it's not or at least they don't use that word). By adding "loaded" that should catch those users who think of these files being loaded and don't think about them being executed.
There was a problem hiding this comment.
All automation/js/.js files are executed sequentially in a single thread. isn't true.
Each rule is run by a dedicated thread, regardless of how the .js files are organized, how many rules are in the same file etc. But, threads aren't the only thing that matters here.
There are also script engine instances and context objects to worry about. Script engine instances now have a global lock that only lets one thread use any given instance at a time. In addition, for SimpleRules, the Action execution is guarded by a lock to protect the context object that is shared amongst all SimpleRules that exist in the same file.
I know that you don't like to care about whether it's a SimpleRule or not, but they behave fundamentally different. For non-SimpleRules, you can have as many as you want in the same *.js file, and they will be independent of each other. For SimpleRules that's not the case, because they implicitly end up sharing the context, which is the context in which the original "build script" runs.
d134386 to
980c865
Compare
|
The purpose of
was to create a contrast: one thread for all js/.js files, but separate thread for each rule. That is: all rules, started from the single thread, implicitly run in many threads.
Adding “loaded” where? Suggest a sentence. I have deleted the Irrespective… line.
How do you interpret #19410 (comment) ? My sentence does not say anything about rules, it speaks only about the The openhab-js utility creates |
Ah, I see what you mean now. It's easy to "read it wrong" though, at least for me 😉
Yes, it's the same misunderstanding as above. The file (aka rule-creation) is executed by a single thread. But the resulting rules aren't. I think it's very difficult to describe this precisely in a way that isn't easy to misunderstand. The way I read "All In a sense, it might be better not to mention threads at all in the documentation, and just say what can and cannot run in parallel - to reduce potential confusion. |
I did
Though based on
I might reword it further to:
The full change would therefor be:
As I said above, a lot of users will be confused because they see the rules being "loaded" from these files. They do not understand that they are "executed" in order to create the rules. And I agree, talking about threads here just brings up a lot of details that end users can't do anything with. |
980c865 to
bd2b3d7
Compare
|
I changed it to:
But the previous sentence is
So it creates a discrepancy between loading a script, without executing it, and executing a script, without loading it. In any case, as it is, is an improvement. |
florian-h05
left a comment
There was a problem hiding this comment.
Please open this PR in openhab-js, as the README in the add-ons repo is mainly a copy from there.
| ### Paths | ||
|
|
||
| For [file based rules](#file-based-rules), scripts will be loaded from `automation/js` in the user configuration directory. | ||
| When many rules are installed from a single `.js` file in this directory, at most one of these rules runs at a time. |
There was a problem hiding this comment.
I would put this sentence into the file based rules section, and rephrase it a bit.
When multiple rules are defined in a single script file, at most one of these rules can run at a time.
And add a note:
When writing rules that extensively query persistence or wait for other I/O, it can make sense to split them across multiple files to allow parallel execution of those.
|
|
||
| For [file based rules](#file-based-rules), scripts will be loaded from `automation/js` in the user configuration directory. | ||
| When many rules are installed from a single `.js` file in this directory, at most one of these rules runs at a time. | ||
| All `automation/js/.js` files are loaded and executed sequentially and alphabetically. |
There was a problem hiding this comment.
This should be the same for everything in the automation folder, as file loading is handled by core and not the add-on.
There was a problem hiding this comment.
Yes, but the other add-ons do not implement Lock when executing eval() in the files. My understanding is that in consequence when many files are present, these are evaluated/loaded/executed in parallel.
There was a problem hiding this comment.
Again, it just doesn't work to "pretend" that SimpleRule isn't a part of the equation. Only SimpleRules have this "one per file" limitation, because they share the context. You can create as many non-SimpleRules as you want from one file, and they will run independently (with different ScriptEngines).
There was a problem hiding this comment.
Can you provide an example, where one .js file installs two rules, which are not SimpleRule, and these rules can work in parallel? My understanding is that JS can create indirectly as many threads for handling javascript code, asit wants, but only one of the thread is executed per ScriptEngine insance.
Also, suggest text for inclusion in README, which you think is right, then the proposed addition can be discussed.
There was a problem hiding this comment.
You can create as many non-SimpleRules as you want from one file, and they will run independently (with different ScriptEngines).
Again, you can't create anything but a SimpleRule with JS. We are not "pretending". SimpleRule is the only part of the equation for JS.
And even then, the only way "normal" rules, if it were possible to create them, could work is if each rule in the file is wholly self-contained, which is not a guarantee. Any reference to any variable not defined in the rule's action itself (e.g. defined at the top of the file, such as the import of items from openhab) is referencing another context (i.e. the context that created the rule in the first place). Do that at the same time from two different rules and you get an exception.
So called "normal" rules are not a Thing for JS. Only SimpleRule is possible at this time. It would be confusing at best to bring it up in the docs for JS something that isn't even possible.
There was a problem hiding this comment.
Again, you can't create anything but a SimpleRule with JS. We are not "pretending".
SimpleRuleis the only part of the equation for JS.
You can create "normal" rules from JavaScript. It took my quite some time to figure out a syntax that works, because I don't use JS scripting and before the documentation wasn't exactly "helpful" for anything but SimpleRule/JSRule (which is the same thing AFAICU), but here it is:
Object.assign(this, require('@runtime'));
Object.assign(this, require('@runtime/RuleSupport'));
const TriggerBuilder = Java.type('org.openhab.core.automation.util.TriggerBuilder');
const ConditionBuiilder = Java.type('org.openhab.core.automation.util.ConditionBuilder');
const ActionBuilder = Java.type('org.openhab.core.automation.util.ActionBuilder');
const RuleBuilder = Java.type('org.openhab.core.automation.util.RuleBuilder');
let rule = RuleBuilder.create('complex_rule').withName('Complex Rule')
.withDescription('A complex rule example')
.withTriggers(
[TriggerBuilder.create()
.withId('aTimerTrigger').withTypeUID('timer.GenericCronTrigger')
.withConfiguration(new Configuration({ 'cronExpression': '0/10 * * * * ? *' })).build(),
TriggerBuilder.create()
.withId('systemStart').withTypeUID('core.SystemStartlevelTrigger')
.withConfiguration(new Configuration({ 'startlevel': 100 })).build(),
TriggerBuilder.create()
.withId('windowOpened').withTypeUID('core.ItemStateChangeTrigger')
.withConfiguration(new Configuration({ 'itemName': 'OpenWindowActive', 'state': 'ON' })).build()
]).withConditions(
[ConditionBuiilder.create()
.withId('timeRange').withTypeUID('core.TimeOfDayCondition')
.withConfiguration(new Configuration({ 'startTime': '14:00', 'endTime': '16:23' })).build(),
ConditionBuiilder.create()
.withId('ephemeris').withTypeUID('ephemeris.NotHolidayCondition').build()
]).withActions(
[ActionBuilder.create()
.withId('preventHeating').withTypeUID('core.ItemCommandAction')
.withConfiguration(new Configuration({ 'itemName': 'LimitedHeatingPower', 'command': '0' })).build(),
ActionBuilder.create()
.withId('announce').withTypeUID('script.ScriptAction')
.withConfiguration(new Configuration({ 'type': 'application/x-javascript', 'script': 'console.info("The window is open, heating turned off");' })).build()
])
.build();
automationManager.addRule(rule);
console.info('Complex Rule added');This is a valid, functioning (if you have the Items) Rule that is not limited by the limitations of SimpleRule. It could have been much easier to write such rules if the "helper lib" was written to utilize it, but it isn't, and neither is the documentation, so it's hard to figure out. That doesn't mean that it "can't be done". It's completely within the realms of what the scripting add-on can do, it's just that you must more or less do it without any "help".
And even then, the only way "normal" rules, if it were possible to create them, could work is if each rule in the file is wholly self-contained, which is not a guarantee. Any reference to any variable not defined in the rule's action itself (e.g. defined at the top of the file, such as the import of
itemsfrom openhab) is referencing another context (i.e. the context that created the rule in the first place). Do that at the same time from two different rules and you get an exception.
I understand what you mean, but I don't know if it's possible to write "normal" rules like that. You can supply the script as a text without a problem, but if you try to make "separate scripts" from functions, you'll meet some obstacles. I don't know if that can be done or not, but it's obviously to get around this that SimpleRule was created. But, it also comes with a lot of drawbacks and limitations.
There was a problem hiding this comment.
Can you provide an example, where one .js file installs two rules, which are not
SimpleRule, and these rules can work in parallel? My understanding is that JS can create indirectly as many threads for handling javascript code, asit wants, but only one of the thread is executed per ScriptEngine insance.
Yes, create one more rule in the same way as the first, in the example in my previous comment.
Also, suggest text for inclusion in README, which you think is right, then the proposed addition can be discussed.
That's hard, because Rule and SimpleRule are blurred together so many other places. Trying to give this a name without a broader explanation would be hard in this context. So, I'd think that the most important thing here is to not establish "facts" that aren't in fact, facts. On "easy way out" could be to just state what is being stated, but use JSRule instead of just applying it generally, e.g.:
"All JSRules in automation/js/.js files are loaded and executed sequentially and alphabetically."
PS! Are they actually executed alphabetically? That sounds strange, locks are usually "random", they will just let some waiting thread through when the lock is released, there is no system to it.
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/rules-and-concurrency/166577/1 |
I create automation/js/a.js with content Are files under
|
bd2b3d7 to
d102a71
Compare
…ot run in parallel
d102a71 to
e262d21
Compare
I thought you were referring to the sequence in which individual rules within the same JS file were being executed. That, given that they share the same lock, will be random I think (if they are all triggered at the same time). So, I misunderstood what you meant. When it comes to the order in which the files are parsed, that is probably likely to be alphabetical. But, I'm pretty sure that this isn't in any way enforced by OH, and is probably OS dependent. What happens in reality is that OH asks the OS for a list of files in a certain folder, and then goes on to process them in the order they are presented in the list. The OS will most likely return the list in whatever is it's "native order", i.e. the same order that Actually, when looking at the code in private CompletableFuture<Void> addFiles(Collection<Path> files) {
return CompletableFuture.allOf(files.stream().map(this::getScriptFileReference).flatMap(Optional::stream)
.sorted().map(this::addScriptFileReference).toArray(CompletableFuture<?>[]::new));
}The file watcher will pick up the changed files and hand them off to different parts of the system, to the scripting add-on in this case. The file watcher itself does this in sequence from a single thread. That doesn't mean that the further processing of those files is done in a single thread. However, when looking in protected ScheduledExecutorService getScheduler() {
return Executors.newSingleThreadScheduledExecutor(new NamedThreadFactory("scriptwatcher"));
}This method can be overridden by subclasses, so that doesn't have to be the case for all add-ons that use As far the rationale for making the choices that have been made, I can't answer. I can only speculate that there have been issues loading the scripts in parallel, and that making it single-threaded has been "easier" than to actually figure out how to solve the problems.
JavaScript syntax and import rules really isn't "my area of expertise". Add JSR223 and access to Java objects into the mix, and I'm reduced to trial and error. So, I can't really say why you get the error you get. But, if you paste the script I made, it should work, and then you can tweak that until it "breaks" to maybe see what causes the error: Object.assign(this, require('@runtime'));
Object.assign(this, require('@runtime/RuleSupport'));
const TriggerBuilder = Java.type('org.openhab.core.automation.util.TriggerBuilder');
const ConditionBuiilder = Java.type('org.openhab.core.automation.util.ConditionBuilder');
const ActionBuilder = Java.type('org.openhab.core.automation.util.ActionBuilder');
const RuleBuilder = Java.type('org.openhab.core.automation.util.RuleBuilder');
let rule = RuleBuilder.create('complex_rule').withName('Complex Rule')
.withDescription('A complex rule example')
.withTriggers(
[TriggerBuilder.create()
.withId('aTimerTrigger').withTypeUID('timer.GenericCronTrigger')
.withConfiguration(new Configuration({ 'cronExpression': '0/10 * * * * ? *' })).build(),
TriggerBuilder.create()
.withId('systemStart').withTypeUID('core.SystemStartlevelTrigger')
.withConfiguration(new Configuration({ 'startlevel': 100 })).build(),
TriggerBuilder.create()
.withId('windowOpened').withTypeUID('core.ItemStateChangeTrigger')
.withConfiguration(new Configuration({ 'itemName': 'OpenWindowActive', 'state': 'ON' })).build()
]).withConditions(
[ConditionBuiilder.create()
.withId('timeRange').withTypeUID('core.TimeOfDayCondition')
.withConfiguration(new Configuration({ 'startTime': '14:00', 'endTime': '16:23' })).build(),
ConditionBuiilder.create()
.withId('ephemeris').withTypeUID('ephemeris.NotHolidayCondition').build()
]).withActions(
[ActionBuilder.create()
.withId('preventHeating').withTypeUID('core.ItemCommandAction')
.withConfiguration(new Configuration({ 'itemName': 'LimitedHeatingPower', 'command': '0' })).build(),
ActionBuilder.create()
.withId('announce').withTypeUID('script.ScriptAction')
.withConfiguration(new Configuration({ 'type': 'application/x-javascript', 'script': 'console.info("The window is open, heating turned off");' })).build()
])
.build();
automationManager.addRule(rule);
console.info('Complex Rule added');Regarding But, I couldn't get any of that to work. When creating the rule you posted above, and querying the REST API for that rule, this is what I get: {
"status": {
"status": "IDLE",
"statusDetail": "NONE"
},
"editable": false,
"triggers": [
{
"id": "c9a13e69-2522-4d43-8dcf-b9752759e8bb",
"configuration": {
"cronExpression": "* * * * * ? *"
},
"type": "timer.GenericCronTrigger"
}
],
"conditions": [],
"actions": [
{
"inputs": {},
"id": "1",
"configuration": {
"privId": "i8",
"type": "application/javascript",
"script": "// Code to run when the rule fires:\n// Note that Rule Builder is currently not supported!\n\nfunction(e){o(e)}"
},
"type": "jsr223.ScriptedAction"
}
],
"configuration": {},
"configDescriptions": [],
"templateState": "no-template",
"uid": "everyMinute",
"name": "Every Minute",
"tags": [],
"visibility": "VISIBLE",
"description": "Logs every minute for testing"
}
Thus, it seems that |
|
Closing in favour of changing the README in openhab-js. |
|
@rkoshak I can't find your comment anywhere, so I'm answering here.
I think I understand what you mean, but it's wrong. It isn't Java at all, it won't compile as Java. It is pure JavaScript, using OH's public API (which is made in Java, but that's irrelevant). It goes to the core of what JSR223 is, it allows you to call APIs across languages. What you construe as Java is simply the structure that is OH, OH core in particular. So, you just can't claim that this "isn't as documented" or "isn't supported". I'd say that if this is somehow "outside" anything, why use JSR223 at all, when it's what it does at its core. The only thing it "goes against" is the way the "helper lib" has developed. As I wrote, this could have been done much easier with a bit of "support" from the helper lib, but for some reason, it has only focused on
I know, and I think it's unfortunate that the same name was picked as what is used in OHs API. I've looked a little bit at it, it seemed to have some support for |
|
I think the reason for JSRule to use SimpleRule (and rule builder is based on JSRule, so it’s using Simple Rule as well) instead of using ScriptActions and ScriptConditions is, that with SimpleRule it’s possible for the callback of the rule to reference anything outside of the callback in the same JS file, making it extremely powerful, as you can maintain shared state across multiple rules, write shared functions that multiple rules can use etc. |
I'm not saying that it doesn't have its merits, and I understand perfectly well why it's used a lot. What I don't "appreciate", is that it seems to have become "the only things that exists" with some of the scripting languages. |
I have been working hard to make sure your statement is wrong ;-) |
I deleted my commend because the issue is closed and I'm tired of arguing about it. I don't care any more. But what I meant was every Class you are using there is Java and JS Scripting makes it clear in the docs one should avoid using raw Java. If we want to make the docs super complicated for the end users covering cases we don't really support and actively recommend against using I won't stand in the way. It would be a net negative contribution to the docs but 🤷♂️ . |
This is follow up of the discussions at openhab/openhab-docs#2565 . The conclusion is that if many rules are installed from many files in
automation/js/.jsthen these rules can run in parallel. But when installing the same rules from a singleautomation/js/.jsfile, then at any time at most one of the rules can be executed.For me it is still unclear, how to test, if transformations can run in parallel and how to see on this differences, when using e.g. JS vs Groovy.