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

Allow to run rules using scheduler to avoid "Failed to execute rule ‘{}' with status '{}'" error path #4072

Open
OlegAndreych opened this issue Jan 28, 2024 · 6 comments
Labels
enhancement An enhancement or new feature of the Core

Comments

@OlegAndreych
Copy link

I propose to use org.openhab.core.automation.internal.TriggerHandlerCallbackImpl#getScheduler when calling scripts to resolve an issue with simultaneous executions, as it is done for "normal" rules execution path.

That's needed for a code reuse using UI scripts.
Many rules are triggered with events with no known intervals and/or timing, so there's no sensible way to spread executions of these rules, so that they don't call scripts simultaneously.

An example of an issue can be found there: https://community.openhab.org/t/simultaneous-calling-of-script-problem/140087.

@OlegAndreych OlegAndreych added the enhancement An enhancement or new feature of the Core label Jan 28, 2024
OlegAndreych added a commit to OlegAndreych/openhab-core that referenced this issue Jan 28, 2024
 Allow to run rules using scheduler to avoid "Failed to execute rule ‘{}' with status '{}'" error path
OlegAndreych added a commit to OlegAndreych/openhab-core that referenced this issue Jan 28, 2024
OlegAndreych added a commit to OlegAndreych/openhab-core that referenced this issue Jan 28, 2024
…{}' with status '{}'" error path (openhab#4072)

Signed-off-by: Oleg Kleshchev <[email protected]>
OlegAndreych added a commit to OlegAndreych/openhab-core that referenced this issue Jan 28, 2024
…{}' with status '{}'" error path (openhab#4072)

Signed-off-by: Oleg Andreych <[email protected]>
@rkoshak
Copy link

rkoshak commented Jan 29, 2024

I commented on the PR because I didn't see this issue at first. Here is a better place to have the discussion.

This might be a breaking change. Right now when one rule calls another rule, the call blocks until the called rule returns.

For example:

console.info('Calling rule');
rules.runRule('longrunning', {}, true);
console.info('Rule returned');

with longrunning

console.info('Long running rule called');
Java.type('java.lang.Thread').sleep(5000);
console.info('Sleep done');

produces the following output:

2024-01-29 11:06:35.142 [INFO ] [nhab.automation.script.ui.scratchpad] - Calling rule
2024-01-29 11:06:35.978 [INFO ] [hab.automation.script.ui.longrunning] - Long running rule called
2024-01-29 11:06:40.997 [INFO ] [hab.automation.script.ui.longrunning] - Sleep done
2024-01-29 11:06:40.997 [INFO ] [nhab.automation.script.ui.scratchpad] - Rule returned

If I understand this proposal that behavior would change and the call to runRule would immediately return. This could break some end user's rules if they depend on this blocking behavior.

@OlegAndreych
Copy link
Author

Hello)

It’s not a change, but an addition.

runNow methods of the org.openhab.core.automation.RuleManager are still in place and work the same way as before.

It’s up to automation scripting addons maintainers/designers to decide how to expose new calling methods (instead or besides the existing) and whether to expose them at all.

My aim is to expose some API for running rules to the Groovy scripting without the need to resort to getting services from the OSGi and without errors on parallel execution.

I thought this PR would be a good first step.

@OlegAndreych
Copy link
Author

Talking about code snippets.

As I’ve said earlier, it’s up to maintainers/designers of an addon to decide how to expose new API.

One way to solve an issue with immediate return and reordering would be to wait for a Future from one of the newly proposed scheduleRun methods to complete.

Awaiting can be done inside the addon and not be exposed to the addon user/script writer.

Another way can be to return Future/Promise from the runScript (or any new additional method/function to expose new calling method) and let user to decide what to do.

As far as I understand, snippets are JS scripting examples. Unfortunately, I’m not familiar with JS scripting addon internals and not confident that there are no pitfalls with proposed strategies to mitigate early return from scheduleRun.

If you have a proposal on how to make an API more convenient or useful - I’m all ears.

@rkoshak
Copy link

rkoshak commented Jan 29, 2024

If this is just additional functionality then I've no concerns. I misunderstood and thought the proposal was to change the current behavior which could cause some problems.

@OlegAndreych
Copy link
Author

In case there's no more objections, can someone approve pull request?

@rkoshak
Copy link

rkoshak commented Jan 31, 2024

I don't have that power. A maintainer needs to come along and review the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature of the Core
Projects
None yet
Development

No branches or pull requests

2 participants