search

openhab / openhab-js

openHAB JavaScript Library for JavaScript Scripting Automation
https://www.openhab.org/addons/automation/jsscripting/
Eclipse Public License 2.0
38 stars 31 forks source link

[items] ItemPersistence: Convert state to primitive type in `persist` method #339

Closed florian-h05 closed 3 weeks ago

florian-h05 commented 3 weeks ago

This fixes an issue, where the asynchronous way persistence services store the passed in state causes a IllegalStateException by GraalJS, because multithreaded access to the script's context is not allowed.

See https://github.com/openhab/openhab-core/pull/4268#issuecomment-2150493510.

florian-h05 commented 3 weeks ago

@jlaur Can you please check if this fixes the issue?

Run npm i git+https://github.com/florian-h05/openhab-js.git#persist-toopenhabprimitivetype inside the automation/js folder and disable injection caching in the add-on settings.

jlaur commented 3 weeks ago

Can you please check if this fixes the issue?

Sure. I'll have a look asap. Will need to figure out how to do this in Windows where I have my development installation.

florian-h05 commented 3 weeks ago

In case you can’t figure it out I can provide you the webpacked version of the library (this is a single file containing the „compiled“ library and dependency code) tomorrow so you can simply download it and put it into the node_modules folder.

jlaur commented 3 weeks ago

In case you can’t figure it out I can provide you the webpacked version of the library (this is a single file containing the „compiled“ library and dependency code) tomorrow so you can simply download it and put it into the node_modules folder.

Thanks. I remembered that I have a (very old) WSL installation on my Windows machine, so I'm currently updating it and will see if I can install nodejs there. Will continue tomorrow and let you know. 🙂

jlaur commented 3 weeks ago

Thanks. I remembered that I have a (very old) WSL installation on my Windows machine, so I'm currently updating it and will see if I can install nodejs there. Will continue tomorrow and let you know. 🙂

That went smoother than expected (although technically it is now tomorrow 😉). I was able to install nodejs and npm after the upgrade, and your npm command worked as well. I have now tested it before and after switching to your new version. I started by reproducing it with the injected version, and after switching to the new version, the exception no longer occurred, and all items were persisted correctly. 👍

florian-h05 commented 3 weeks ago

after switching to the new version, the exception no longer occurred, and all items were persisted correctly. 👍

Thanks for the feedback, great!

I will merge this PR now and try to have a look at persisting TimeSeries, however I suspect this might be tricky due to the asynchronous nature of persistence services persisting data - we have to avoid that persistence gets something that is referenced by the script context (I think so, to be honest I haven’t found docs wrt to this and therefore it’s more or less guessing from my side - but your error message and this fix clearly indicate that I’m right).