Threshold Alert and Open Reminder [4.0.0.0;5.1.9.9]

Add-on Marketplace Rule Templates
,
202

This is on the “todo” list. I’ve just not gotten around to it. I’ve been handling this through expire and my “restart expire” rule template. That rule fires on system started and updates an Item with it’s restored state. Because the rules are running at this point instead of when the Item was first restored, it kicks off the threshold alert rules in addition to restarting the expire timers.

Most of my free time these days have been used up by eliminating technical debt built up over a decade in my home lab. I’m also waiting for the new YAML format to be supported by the marketplace so the development process will become much improved (I can modify and test locally and then upload the actual file that I tested with instead of what needs to be done now).

But soon I intend to return to my rule templates and rework them and add this feature in. I’ll take a look at the PR though and merge it if it doesn’t prevent me from achieving one of my other todos in the future. I really appreciate the contribution!

Edit: I’ve reviewed the PR. There are a few changes requested but overall it looks good. Thanks!

203

Hey @rlkoshak,

Thanks for the comments and good catch. I addressed your comments and updated the pull request.

Cheers,
Flo

204

PR is merged and first post is updated.

The upgrade procedure is the same as usual:

  1. Navigate to Add-on Store → Automation and removed the Threshold Alert and Open Reminder rule template, refresh, then readd to pull the latest template.
  2. Navigate to Settings → Rules
  3. Filter on the tag “Marketplace” and find all the ones with the “Marketplace:144863” tag (these are the rules created from, this tempalte).
  4. Click the purple “Regenerate” button in the bottom right.

If you’ve rules instantiated from the template prior to OH 5.0 (or you are running a version of OH prior to that) you will need to delete the old rule and create it anew. Open the code tab and copy the “Configuration” section to to remember the settings you made for that instance of the rule so you can recreate it with the same properties.

205

Hey Rich,

upgrading to 5.2. I found two bugs that prevented the rule from restoring an old state on reboot. I created a PR.

Cheers,
Flo

206

Thank you. I’m having some issue with it calling my processing rule at the moment too. But it’s not consistent so I’m not sure yet what the problem is. I’m going to also migrate this over to thenew YAML file format and to use the wrapper and JS event Object as well. So I’ll merge your PR first but stay tuned for a new version for OH 5.2+ sometime in the next week too.

207

Watch out for the “new behavior” when running one rule from another in JS. Because of improved locking and the use of the rule thread, some things that “would work” (although not safely) in the past won’t anymore. You can also increase the lock acquisition timeout so that the rule tries for longer before giving up, if it needs to wait for something else to finish.

When @florian-h05 implements runAsync() in the helper library, I think some of these situations will become easier. In cases where you don’t need the result, you only want to trigger the rule, you can use that instead, which makes the whole threading/locks cabal much simpler.

208

The called rules only take a few milliseconds to finish and return. All they do is send a notification. But the called rule is passed data as arguments from the calling rule which is what I fear is the problem. (Meaning that passing data to rules have to follow that same rules as storing data in the shared cache: only Java and primities are supported).

This wasn’t a problem when the called rule was run from the same thread but my understanding is now it runs in it’s own thread.

If that turns out to be the case, it breaks all kinds oof stuff in this and several other of my templates and it will require my rethinking the whole paradigm or switching to jRuby for these tempaltes. The biggest problem is I don’t know ahead of time how many “things” I need to pass to the called rule. So I need to use some sort of structure (e.g. Array).

But except for noteing that my rule is throwing multi-threaded exceptions where it wasn’t before, I’ve not looked into it any farther.

209

The rule now run in the rule’s dedicated thread, instead of the thread of the calling thread, which was always a “flaw”. The whole point in having dedicated threads for each rule is to prevent them from having “concurrency issues with themselves”, making it impossible for a rule to run in parallel with itself. By use the rule’s thread, this now also works for runRule(), in the past it was a matter of “luck” if it happened to run in parallel or not. If a rule isn’t triggered a lot, it was likely to work without “collision” though.

But, this change in itself doesn’t explain the Graal multi-threaded exceptions. Sending JS objects between JS rules will be a problem, and this is dictated by GraalJS. It is GraalJS that enforced this, and the whole “rule” is a bit strange in that it only applied to JS objects, not Java objects. There might be some deeply technical reason for the choice, I don’t know, but there’s not much we can do about that, except to go back to the other previous system where rules run in the “wrong” thread if called using runNow().

The limitation with JS objects only applies if there are “GraalJS on both sides”. So, if you send JS objects to a core action, it shouldn’t matter - the same goes if you send it to a different scripting language. But, if you send it from one GraalJS script to another, it will object.

I imagine that the helper library could simplify this a lot, if it created a way to “sanitize parameters”. It could just loop through whatever it was given, replace JS objects with Java Maps, JS Strings with Java Strings etc., recursively. It wouldn’t be able to handle special JS “classes” where there’s nothing to map it too, but I imagine that mapping objects → maps, arrays → arrays, strings → strings recursively, quite a lot would be solved..?

210

OK, then I need to completely rethink these rule templates and this change should have been reported as a breaking change.

Frankly, the limitation to primitives is far too strict to make this remotely usable for me. I don’t expect a change here as the consensus when I complained about the same issue with the shared cache was “you shouldn’t put anything but primities into the shared cache in the first place” so :person_shrugging: . The problem isn’t that this use to work and now it doesn’t. The problem is I shouldn’t have been doing that in the first place. It’s my fault.

Maybe it’s time to go learn some jRuby instead.

211

The problem is also that it’s very difficult to know what is being done. I had no idea that this would “break” anything, because I no idea that people sent JS objects between rules, and frankly, even if they did, I thought that the JS scripting lock would prevent GraalJS from complaining (but I realize now that the lock doesn’t cross rule boundaries, so it won’t).

But, it has been in the snapshots for a while, and there has been no complaints about the problem before the last milestones/release candidates. There might still be something I’m missing here, it’s still not entirely clear to me exactly what the “rules” are for GraalJS.

I think it’s too early to start any massive transition, there might still be things we can do that will solve most issues transparently for the users.

edit: Also, from what I understand, this also applies to JS strings..? That is a really strange and unexpected complication if that’s the case, because in Java, strings are immutable and can be shared across threads without problems. If the JS string created by GraalJS isn’t immutable and does trigger the “multi threaded access” situation, you have a situation that is much more cumbersome to handle than what it would be in Java.

Ultimately, JS, Ruby and Python all face the same challenge: These are single threaded languages that are being used in a multithreaded environment. Since they are “natively single threaded”, they don’t have the built-in tools to handle multithreading. At the same time, an application like OH would never have worked without multithreading, in addition to being much slower, any one bug that makes something hang would force “everything” to hang. A rule using sleep() would make the whole system sleep, not just the rule.

It’s not entirely clear to me what the philosophy is around threading and these scripting languages, from the makers. It looks like they basically just leave it to the “consumers” of the scripting languages to figure out. GraalJS has probably introduced the “hard fail” when multiple threads access the same object because without it, there was too much mayhem with people that think in “one dimension”/single threaded started wreaking havoc in a multithreaded environment. People will naturally blame GraalJS for being “buggy”, and trying to explain what is really going on won’t work. By implementing the “rule” the way they have done now, they make avoid the “blame” for this, but they also prevent a lot of situations that would most likely have had no ill effects (despite not being 100% “safe”) from working.

Perhaps OHs whole approach to these single-threaded languages are wrong. Perhaps we shouldn’t use the “rule threads” for executing anything in these languages, perhaps instead there should be one “GraalJS thread” that executed everything GraalJS. It would come with some caveats like worse performance and the potential for one “bad rule” to make everything else stop, but it would eliminate the possibility of ever seeing the “multithreaded access” exception, and it would allow getting rid of the complicated locking system.

For this to “work” at all in JS, it has the event loop, promises and all that. It is all made so to basically make it impossible for code to be blocking. You can’t pause execution of JS, you must create a timer or a promise that is executed at a later time instead. This is to prevent the situation where one “bad” piece of code can lock up the single thread. But, here, this isn’t a viable approach, because you can call Java functions that are blocking. Which undermines the whole “workaround” that is designed to prevent single-threaded execution from bringing everything to a halt.

So, I see this as a difficult situation no matter how you twist it.

212

What type of data is passed? The documentation said this for 5.1.x:

args optional dict of data to pass to the called rule

openhab-js in 5.2.0 converts the passed in dict to a Java HashMap, however non primitive values in the dict pose a problem. The dict as an object itself however is handled by openhab-js.

If you tell me what you need to pass we can try to add additional conversions in openhab-js for these from JS to Java. Depending on what Java type it is, the JS rule that receives it can handle it like native JS, GraalJS e.g. allows .key access on HashMaps. However that whole topic isn’t easy for us developers, we don’t know what people will do outside of what we thought of …

Primitives and Java objects, which allows for timers as created by createTimer …

213

I would suggest to expand this conversion to work recursively, and to convert everything that “can be mapped”. That means at the very least maps, strings and arrays - and anything else we can think of that has a predictable mapping from JS to Java.

edit: I would also expose the conversion routine as a function in the helper library, so that it can be called “manually” e.g. when you want to place something in the shared cache.

214

Before GraalJS, we had Nashorn (and Rhino), and I believe I even read somewhere that JS run by those could behave very unpredictable when being accessed from multiple contexts.
I am quite sure GraalJS devs didn’t want to repeat that experience, and decided that JS as a single-threaded language doesn’t allow for multi-threaded access in Graal

If JS doesn’t have the possibility to call blocking Java methods (which is the case in plain Node.js applications), it is able to have quite a good performance for a single-threaded language thanks to embracing async that much …

Strings shouldn’t be a problem I think. Arrays could be converted to ArrayList, no idea how that would look on the receiving side though …

215

Neither I nor apparently any of my users were running with the snapshots or milestones I guess. My time was short so I couldn’t do as much testing as I usually do leading up to a release.

When this rule calls the processing rule it passes the following:

Variable Purpose
alertItem name of the Item that generated the alert
alertState current state of the Item
isAlerting boolean, when true the Item meets the threshold comparison, when false the Item exited the threshold state
isInitialAlert boolean, when true the Item just now met the threshold comparison, when `false, the Item has been in this state for awhile.
threshItems JavaScript Array of JavaScript Item Objects whose state meet the alerting criteria
threshItemLabels JavaScript Array of the Item labels whose state meet the alerting criteria
nullItems JavaScript Array of JavaScript Item Objects whose state is NULL or UNDEF
nullItemLabels JavaScript Array of the Item labels whose state is NULL or UNDEF

I pass JavaScript stuff so that Blockly users can handle the data that’s received without resorting to a code block to convert it back to a usable form.

The code is:

        var callRule = (state, ruleID, isAlerting, isInitialAlert, record) => {

          if(ruleID == '') {
            console.debug('No rule ID passed, ignoring');
            return;
          }

          console.debug('Calling ' + ruleID + ' with alertItem=' + record.name + ', alertState=' + state + ', isAlerting='
                        + isAlerting + ', and initial alert ' + isInitialAlert);
          var rl = cache.private.get('rl', () => RateLimit());

          const throttle = () => {
            const gk = cache.private.get('gatekeeper', () => Gatekeeper());
            const records = cache.private.get('records');
            gk.addCommand(record.gatekeeper, () => {
              try {
                // If the Item hasn't triggered this rule yet and therefore doesn't have a record, skip it
                const allAlerting = items[group].members.filter(item => records[item.name] !== undefined && isAlertingState(stateToValue(item.state), records[item.name]));
                const alertingNames = allAlerting.map(item => item.label);
                const nullItems = items[group].members.filter(item => item.isUninitialized);
                const nullItemNames = nullItems.map(item => item.label);
                rules.runRule(ruleID, {'alertItem':        record.name,
                                       'alertState':       ''+state,
                                       'isAlerting':       isAlerting,
                                       'isInitialAlert':   isInitialAlert,
                                       'threshItems':      allAlerting,
                                       'threshItemLabels': alertingNames,
                                       'nullItems':        nullItems,
                                       'nullItemLabels':   nullItemNames}, true);
              } catch(e) {
                console.error('Error running rule ' + ruleID + '\n' + e);
              }
              console.debug('Rule ' + ruleID + ' has been called for ' + record.name);
            });
          }
          // Only rate limit the alert, always make the end alert call
          (isAlerting && record.rateLimit !== '') ? rl.run(throttle, record.rateLimit) : throttle();

        }

record.name is a String
state is a String
isAlerting and isInitialAlert are both booleans

You can see where the rest come from in the code above.

That was my main concern and for Blockly in particular. Otherwise I could use a map to pull the rawItem and use utils.jsArrayToList() before adding it to the dict. But then Blockly users wouldn’t be able to do anything with it.

I could probably get by with just passing an array of Strings here. The Item Objects probably should be pulled again in the called rule anyway. But based on my experiences with the shared cache, I would not expect that to work either.

216

That depends on what you consider “good performance” I guess. You can never utilize the hardware properly, especially these days with many cores, and there are other aspects of hardware that you can’t benefit from when it comes to data transfer etc. I tend to avoid things made in Node, Python etc. as much as possible, I always find them “sluggish” and poorly performing. That doesn’t mean that everything done in these languages are “slow”, that depends entirely on the task. When it comes to OH rules, they should be plenty fast most of the time, and because GraalJS compiles them to bytecode, probably even faster than their “native versions”. But, still there are challenges as discussed here…

It needs to be tested 100% that strings aren’t a problem. I’m not so sure, because we know that there’s a difference between a “JS string” and a “Java string”. I suspect that they have hooked their little “enforcement” to all JS objects, which would then probably include strings.

Java have both array and List. List is more convenient to work with, so it would probably be the better choice, but we also have arrays. Because we can’t know the types that will be put into them, that would probably mean that they’d have to be object[]. That’s not terribly convenient to work with, but it is what it is. You’ll have the same “problem” with Lists unless you make the conversion routine really “advanced” and first figures out if all elements are the same type, they would have to be List<Object> aka List<?>.

When the helper lib converts the incoming data back to JS objects, doesn’t it map both Lists and arrays to JS arrays?

I have no doubt that the situation was something like this, but this also means that e.g. JRuby still does it “like Nashorn did” and basically just hopes for the best. When it comes to GraalPy, I’m not sure if it’s as strict as GraalJS or not, but the challenge is basically the same for all of them. Either they will allow you to do things that will bite, or they refuse to do it, making the problem appear “upfront” instead of “perhaps later”. In a way I agree with what GraalJS has done, but I wish that there were some more “finesse” to it, for example that you could make an object immutable/make an immutable copy, that then wouldn’t be affected by the enforcement.

It’s entirely pointless for me to speculate how it could have been solved though, it will have zero impact on what is done anyway. But, the whole situation is a challenge no matter which way you look at it.

217

Gemini claims that my suspicion about JS strings is true:

GraalJS strictly enforces the “one thread at a time” rule for all native JavaScript values (including JS strings) when wrapped or accessed via the Truffle/Polyglot Interoperability API.

More Gemini claims, but I suspect that this could be true:

No, you cannot explicitly tell GraalJS to flag a native JavaScript object as “safe for concurrent multi-threaded access,” even if you use Object.freeze(). [1]

The restriction is not strictly about object mutability; it is an architectural limitation of the underlying GraalJS execution engine. GraalJS optimizes property access using “hidden classes” (shapes) and internal metadata caches that are completely thread-unsafe. Allowing two threads to access native JS objects concurrently—even read-only frozen ones—could corrupt these internal engine caches.

If the above is true, there is no other way than to convert everything to Java objects, to get rid of these internal engine metadata that are attached to the objects.

218

Given that’s the case, there needs to either be automatic coversion back to JS Objects on the callee side, or blocks need to be added to Blockly to allow hem to convert them to JS Objects manually.

219

@florian-h05 While somewhat off-topic here (because implementing it would mean a huge reorganization), Gemini claims that there is a context.enter() and context.leave() functionality in GraalJS that actually allows other threads access to JS objects as long as they have “entered the context” of the originating script. From what I understand, this is an exclusive lock that also serves as a “regular lock” preventing threads from entering. If this is true, replacing the current “engine lock” with the built-in “context lock” might make some things possible that aren’t today. But, I don’t know about the details to say for sure, it’s just something that might be worth looking more closely at.

220

I’ve made a suggestion for how I think this could be handled in principle:

This function probably shouldn’t live in rules.js, and it might be beneficial if it’s automatically called in other places as well, like when putting something into the shared cache.

As for converting the other way, something similar could be done, but I think that would need to be called explicitly by the user, so that it’s possible to retain Java objects where that’s desirable.

221

Gemini‘s claims are actually correct, these methods are also in the JavaDoc. However I don’t see how they could help where locking couldn’t … runAsync + locking on the „callers“ lock when passing data to the called should solve the issue I think. We could overwrite the RuleManager inside the add-on and add some locking on the callers lock there. A bit similar to how SimpleRules are wrapped and locking is added when they are created from JS.