[automation] Enable asynchronous execution of rules and lock engine lock when loading scripts

[Nadahar]

This has been in the back of my mind for a little while. The situation is that when runNow() is called, the calling thread is forced to wait for the rule execution to finish before returning, even if it doesn't need the execution result. This causes unnecessary waiting and potential deadlocks.

This PR offers runAsync() in addition to runNow(), where the caller can choose which behavior is desirable. runAsync() returns a Future, so that it's still possible to wait for and retrieve the result, but it's entirely optional. It's also possible to cancel the rule execution if it hasn't yet started.

I did this somewhat in a rush now, because I saw that this would also solve what #5634 does without the need to schedule yet another set of "tasks" that will just have to wait while the rule thread executes the rule. I'm thus submitting it as a draft, it's not "polished" and there might be aspects I have overlooked.

I'd like the approach to be discussed before investing further work in the idea.

[florian-h05]

I think this additional capability, as well as executing start level triggers asynchronously, is a good thing 👍
I also think this is a better solution than #5634 as it is more flexible and IMO architecturally more sophisticated.

[Nadahar]

If there is a desire to for this solution, I want to ask if anybody sees something obvious that I've missed. ModuleHandlerCallback also have the two runNow() methods, I have not added the async variants there because it's yet unclear to me what purpose runNow() serves there. This is one thing that needs to be figured out.

It is used by RunRuleActionHandler (the predefined action that runs another rule), and it doesn't need "async" because it returns a result. It is also used by SimpleTriggerHandlerCallbackDelegate, which I'm not sure what does. It looks like it might be a part of support for scripted triggers, something that I don't think was ever finished? The JavaDocs state that: The {@link SimpleTriggerHandlerCallbackDelegate} allows a script to define callbacks for triggers in different ways.

[Nadahar]

I've pushed another commit here that isn't really related. I can make a separate PR for it if desired, however, having everything spread across many PRs/branches makes it very hard to test how things work together.

What this commit does is make sure that the lock for script engines that implement Lock is locked before loading a script file. This is necessary to prevent rules that are created by the script from start running while the script is still being executed. I really don't have a good position to test this properly, so something must be figured out.

@florian-h05 I'm hoping that the lock I lock is the correct one. I'm not quite sure exactly how the script engine instances (that are really just script contexts for Graal) are managed, but since the rules will later have access to the "shared context" created by the script, I have some hope that this lock is the correct one.

[Nadahar]

As an option, I've pushed a commit that implements the asynchronous methods in ModuleHandlerCallback as well. I have no idea if that is useful or not, because I'm not really sure what ModuleHandlerCallback does. But, it already contains the runNow() variants that are found in RuleManager, so it might make sense to "mirror" the new methods as well. I simply don't know, but it's hard to imagine that it would hurt.

[Nadahar]

Regarding ModuleHandlerCallback, I've given figuring it out another try, but as often is the case with everything rule engine, all I achieve is to end up in this huge PR: eclipse-archived/smarthome#4468

I find it "impossible" to figure out exactly what that PR changed and why, but this is where the runNow() methods made it into ModuleHandlerCallback. The purpose is still unclear to me, which makes deciding if runAsync() should be there as well very difficult. I can't evaluate something I don't understand.

[digitaldan]

I'm good with this approach 👍

[florian-h05]

I am also good with this approach 👍

I think we should get this finished before doing another milestone build on Friday, this is required to fix some multithreading issues people have been experiencing. @Nadahar You know what I refer to.

[florian-h05]

Probably the PR title and description should be adjusted as this also ensures that script loading is synchronized if needed.

[Nadahar]

@florian-h05 I absolutely agree that this should be in before the next milestone. But, we must figure out how to handle the lock acquisition timeouts. Currently, they are hardcoded inconsistently.

Having a long timeout will make things go very slow before finally failing, if things happen rapidly, the "lock acquisition queue" could grow infinitely long because new entries are added faster than they time out. That would effectively grind the system to a halt, at least everything related to these rules. So, while technically not "hanging" or "deadlocked", the effect would be indistinguishable. At the same time, a too short timeout might cause problems for some users that keep triggering/running rules faster than they can finish. One can discuss whether that's a problem or not, because the "design" of the rules are hardly ideal if this is the case. Yet, if people have been used to this "working somehow", but now it results in an error, they might find that "unacceptable" and trying to explain that they must rearrange their logic might not be an easy task.

So, what is the right value? Should it be configurable?