Script Engines now removed from manager when closed

[jpg0]

Remove script engines from managed once they are closed. It looks like this is a longstanding bug that was only recently uncovered by #2681.

Signed-off-by: Jonathan Gilbert jpg@trillica.com

[digitaldan]

LGTM , fyi i have tested this on my system and it does indeed solve the issue.

[digitaldan]

I think this is not related to this issue specifically, so should not hold up merging this PR, but @jpg0 scripts are not being loaded when OH starts, and i never see [.rulesupport.loader.ScriptFileWatcher] - Loading script... in the logs. Scripts only load when they are updated/changed after the binding is loaded. (so running touch on a file will load it). I'm not sure if this lives in core or in the addon as i have not started to track it down.

[wborn]

Thanks!

[jlaur]

@jpg0 - do you think this PR with openhab/openhab-addons#12022 is stable and would be worth fixing in a 3.2.x version? In this case we would consider cherry-picking your commits from these two PR's. From the forums, opened issues and my own experience, these bugs seems like showstoppers for actually using jsscripting in 3.2. Please confirm also if you agree to this or not.

[jpg0]

@jlaur I believe that they're stable, yes. I do remember some interaction with UI rules however though, where script engines are currently re-used - maybe @digitaldan is more across this?

When is 3.3 scheduled to be released? I see that M1 is out and this fix is certainly in that.

[rkoshak]

3.3 should be release around the end of June assuming OH follows the usual release cadence.

[digitaldan]

@jlaur Its a good point, and June does seem like a long time. @wborn @kaikreuzer WDYT about these changes in a 3.2.x patch release? Is that a possibility ?

[jlaur]

Its a good point, and June does seem like a long time. @wborn @kaikreuzer WDYT about these changes in a 3.2.x patch release? Is that a possibility ?

Exactly. I believe I managed to convince @kaikreuzer as he already created the 3.2.x branch. :) So wanted to confirm with you if you agree that this bug is critical and that the fix is low risk. In this case we can go ahead and cherry-pick the commits into the 3.2.x branch.

[jlaur]

@digitaldan and @jpg0 - can you confirm that cherry-picking these commits would be safe and fix the issue with jsscripting out of memory issue in 3.2:

• 59bc10a
• openhab/openhab-addons@4d88ad0

[jpg0]

@digitaldan there is no issue regarding engine reuse for UI scripts related to this is there?

Other than that there is nothing that I'm aware of that would be unsafe in these commits.

[digitaldan]

there is no issue regarding engine reuse for UI scripts related to this is there?

Do you have a specific issue or PR for that? I know this sounds familiar, but honestly i'm having trouble remembering it.

[jpg0]

Hmm, I had a look but can't find anything. If it was a problem, it would show up immediately though, so as long as there is basic testing (create a rule in the UI and run it, edit it and run again), then we'd catch it.

[jlaur]

@kaikreuzer - I don't have access to openhab-core, but I think the fix for core in 3.2.1, if we want to proceed, would be:

• git checkout 3.2.x
• git cherry-pick 22c2859
• git cherry-pick 59bc10a

Then in openhab-addons (modified to provide link):

• git checkout 3.2.x
• git cherry-pick openhab/openhab-addons@4d88ad0

FYI @jpg0, @digitaldan