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

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

Merged

Conversation

florian-h05
Copy link
Contributor

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 openhab/openhab-core#4268 (comment).

…ve type

This fixes an issue,
where the asynchronous way persistence services store caused multithreaded access to the script's context.

Signed-off-by: Florian Hotze <[email protected]>
Signed-off-by: Florian Hotze <[email protected]>
@florian-h05
Copy link
Contributor Author

@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.

@florian-h05 florian-h05 added the bug Something isn't working label Jun 6, 2024
@florian-h05 florian-h05 added this to the to be released milestone milestone Jun 6, 2024
@jlaur
Copy link
Contributor

jlaur commented Jun 6, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

jlaur commented Jun 6, 2024

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
Copy link
Contributor

jlaur commented Jun 6, 2024

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
Copy link
Contributor Author

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).

@florian-h05 florian-h05 marked this pull request as ready for review June 7, 2024 05:07
@florian-h05 florian-h05 requested a review from a team as a code owner June 7, 2024 05:07
@florian-h05 florian-h05 merged commit 89692b1 into openhab:main Jun 7, 2024
4 checks passed
@florian-h05 florian-h05 deleted the persist-toopenhabprimitivetype branch June 7, 2024 05:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants