feat(script): add repeat interval for script failure and exec-if

[indev29]

What type of PR is this? (check all applicable)

• Refactor
• Feature
• Bug Fix
• Optimization
• Documentation Update
• Other: Replace this with a description of the type of this PR

Description

Added repeat interval for script failure (interval-fail) and exec-if (interval-if), both falling back to interval if not specified. The implementation is straightforward, current configurations retain their behaviour.

Related Issues & Documents

Closes #943, #2606

Documentation (check all applicable)

• This PR requires changes to the Wiki documentation (describe the changes)
• This PR requires changes to the documentation inside the git repo (please add them to the PR).
• Does not require documentation changes

The following should be added to Modules/script/Basic Settings:

; Interval used when the `exec` command returns with a non-zero exit code 
; If not defined, interval is used instead
interval-fail = 300

; Seconds to sleep between `exec-if` invocations
; If not defined, interval is used instead
interval-if = 180

Additional notes

One thing I've discovered is that the whole exit-code mechanism is not very reliable due to the fact that script invocations and output updates are not synchronized in any way. Here's a sample config to describe the problem:

[bar/bar]
modules-right = test

[module/test]
type=custom/script

format = <label>
format-background = #090
format-foreground = #fff
format-padding = 4

format-fail = <label-fail>
format-fail-background = #900
format-fail-foreground = #fff
format-fail-padding = 4

label = %output%
label-fail = %output%

exec=echo %counter% $(date +%S) ; ((%counter% % 2))

interval=3
interval-fail=5

The script prints it's %counter% with date in seconds and fails every other time. Expected output is green-red-green-red....
Here's an example of the output I've got:

1 00 (green)
2 03 (green)
3 08 (red)
4 11 (green)
5 16 (green)
6 19 (red)
7 24 (red)

As you can see 2 was considered "successful" by formatting, but applied the correct inteval, because it's actually controlled by script_runner. What actually happened is this:

set exit_status: 0 // exec at ~00
// 1
get_output
  get exit_status: 0
// 2
get_output
  get exit_status: 0
set exit_status: 1  // exec at ~03
// 3
get_output
  get exit_status: 1
set exit_status: 0 // exec at ~08

The same is for 4-5 and 6-7.

My point is, it may be confusing for users if their scripts go for (probably longer) interval-fail while being displayed as successful. It's also not an expected behavior really and performs poorly with "flickering" scripts like the one in the example. At the same time I'm not sure If we could do something about it.

Sorry for not noticing this before #2596.

[codecov]

Codecov Report

Merging #2630 (691b04c) into master (8db3e04) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

❗ Current head 691b04c differs from pull request most recent head 837c2f7. Consider uploading reports for the commit 837c2f7 to get more accurate results

@@            Coverage Diff             @@
##           master    #2630      +/-   ##
==========================================
- Coverage   11.90%   11.89%   -0.01%     
==========================================
  Files         153      153              
  Lines       11318    11320       +2     
==========================================
  Hits         1347     1347              
- Misses       9971     9973       +2     

Flag	Coverage Δ
unittests	11.89% <0.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/adapters/script_runner.cpp	0.00% <0.00%> (ø)
src/modules/script.cpp	0.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8db3e04...837c2f7. Read the comment docs.

[patrick96]

Looks good! Thanks 😃

[patrick96]

As for the timing inconsistencies, you're right, the updates not being synchronized with the script runner are an issue. The intervals themselves are all accurate because this is all running synchronously, the updates are the issue.

I think the following solution could work:

The script module shouldn't should arbitrarily call the get_* functions on the script_runner whenever it wants to update.
Rather, the module should fetch those values in the on_update callback and store them locally. Or even better, the script runner collects all data into some data structure and passes it to the on_update callback. Then we could remove all the data related functions from the runner and the module would only interact with it to control its behavior.

This way, we make state-changes in the script module explicit (everytime the callback is called) and the script module doesn't read inaccurate data (in your example, the script module read the wrong exit code because the on_update callback was called before the exit code was determined).

What do you think about this? Do you want to tackle this?

[indev29]

What do you think about this? Do you want to tackle this?

I think I should, yeah. I'll make a separate PR for these changes.

[patrick96]

Appreciate it 🙂

[patrick96]

@indev29 If you work on it, use the hotifx/3.6.2 branch as the starting point so that we can more easily get it into the next hotfix release.

There has already been a potential report of this issue (even without the intervals): #2650