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

PersistenceExtensions: Support state as string for persist method #4268

Merged
merged 1 commit into from
Jun 5, 2024
Merged

PersistenceExtensions: Support state as string for persist method #4268

merged 1 commit into from
Jun 5, 2024

Conversation

florian-h05
Copy link
Contributor

This is a very useful addition for scripts & rules, e.g. for JS Scripting because in JS Scripting no native (Java) types are used (nearly everything is converted to JS tyes).

Copy link
Contributor

@mherwege mherwege left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@J-N-K J-N-K left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! useful addition.

@J-N-K J-N-K added the enhancement An enhancement or new feature of the Core label Jun 5, 2024
@J-N-K J-N-K added this to the 4.2 milestone Jun 5, 2024
@J-N-K J-N-K merged commit f7f4b76 into openhab:main Jun 5, 2024
4 checks passed
@florian-h05 florian-h05 deleted the persistenceextensions-persist-string branch June 5, 2024 16:32
@jlaur
Copy link
Contributor

jlaur commented Jun 5, 2024

@florian-h05 - I'm not sure if related, but will this fix the following issue?

With this rule snippet (where spotPrice is a PersistedItem and spotPrice is a Quantity):

items.Energi_Data_Service_Total_Price.persistence.persist(spotPrice.timestamp, totalPrice);

I get this (with JDBC fixed by openhab/openhab-addons#16845):

openhab> SLF4J: Failed toString() invocation on an object of type [jdk.proxy11.$Proxy188]
Reported exception:
java.lang.IllegalStateException: Multi threaded access requested by thread Thread[OH-jdbc-1,5,main] but is not allowed for language(s) js.
        at com.oracle.truffle.polyglot.PolyglotEngineException.illegalState(PolyglotEngineException.java:129)
        at com.oracle.truffle.polyglot.PolyglotContextImpl.throwDeniedThreadAccess(PolyglotContextImpl.java:1034)
        at com.oracle.truffle.polyglot.PolyglotContextImpl.checkAllThreadAccesses(PolyglotContextImpl.java:893)
        at com.oracle.truffle.polyglot.PolyglotContextImpl.enterThreadChanged(PolyglotContextImpl.java:723)
        at com.oracle.truffle.polyglot.PolyglotEngineImpl.enterCached(PolyglotEngineImpl.java:1991)
        at com.oracle.truffle.polyglot.HostToGuestRootNode.execute(HostToGuestRootNode.java:110)
        at com.oracle.truffle.api.impl.DefaultCallTarget.callDirectOrIndirect(DefaultCallTarget.java:85)
        at com.oracle.truffle.api.impl.DefaultCallTarget.call(DefaultCallTarget.java:102)
        at com.oracle.truffle.polyglot.PolyglotObjectProxyHandler.invoke(PolyglotObjectProxyHandler.java:103)
        at jdk.proxy11/jdk.proxy11.$Proxy188.toString(Unknown Source)
        at org.slf4j.helpers.MessageFormatter.safeObjectAppend(MessageFormatter.java:291)
        at org.slf4j.helpers.MessageFormatter.deeplyAppendParameter(MessageFormatter.java:263)
        at org.slf4j.helpers.MessageFormatter.arrayFormat(MessageFormatter.java:225)
        at org.slf4j.helpers.MessageFormatter.arrayFormat(MessageFormatter.java:160)
        at org.ops4j.pax.logging.slf4j.Slf4jLogger.debug(Slf4jLogger.java:412)
        at org.openhab.persistence.jdbc.internal.JdbcMapper.storeItemValue(JdbcMapper.java:216)
        at org.openhab.persistence.jdbc.internal.JdbcPersistenceService.internalStore(JdbcPersistenceService.java:174)
        at org.openhab.persistence.jdbc.internal.JdbcPersistenceService.lambda$3(JdbcPersistenceService.java:157)
        at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:539)
        at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
        at java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:304)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
        at java.base/java.lang.Thread.run(Thread.java:833)
Caused by: Attached Guest Language Frames (1)

Whereas the following works:

var { QuantityType } = require("@runtime");
const ZonedDateTime = Java.type('java.time.ZonedDateTime');

var state = new QuantityType(totalPrice.toString());
var timestamp = ZonedDateTime.parse(spotPrice.timestamp.toString());
items.Energi_Data_Service_Total_Price.persistence.persist(timestamp, state);

@jlaur
Copy link
Contributor

jlaur commented Jun 5, 2024

@florian-h05 - I just repeated the test with a newly built org.openhab.core.persistence-4.2.0-SNAPSHOT.jar including your enhancement, and the problem persists. If I change the line (recap: totalPrice is Quantity):

items.Energi_Data_Service_Total_Price.persistence.persist(spotPrice.timestamp, totalPrice);

to:

items.Energi_Data_Service_Total_Price.persistence.persist(spotPrice.timestamp, totalPrice.toString());

it works as expected.

So I'm guessing this will be fixed when openhab-js is refactored to use the new overload taking state as string?

@florian-h05
Copy link
Contributor Author

florian-h05 commented Jun 5, 2024

I get this (with JDBC fixed by openhab/openhab-addons#16845):

The problem is (as the message indicates), that the persistence service persist asynchronously and attempts to access the passed in Quantity, which itself is owned by the GraalJS ScriptEngine - this then causes the thrown exception because GraalJS doesn’t allow multi-threaded access (for good reasons) and still runs in the script thread.
Most entry points to the script (timers, rule callback execution) are synchronised by the add-on, but in this case this is not possible that way.

So I'm guessing this will be fixed when openhab-js is refactored to use the new overload taking state as string?

Exactly. As primitives aren’t passed by reference, we can fix the multi-threading issue by converting the Quantity to string, pass that to core, which then creates a new QuantityType from it and passes that one to the persistence service.

openhab-js already contains the code for the to string conversion (already done for postUpdate and sendCommand), I just need to use it as well for the persist call.

@florian-h05
Copy link
Contributor Author

@jlaur See openhab/openhab-js#339.

florian-h05 added a commit to openhab/openhab-js that referenced this pull request Jun 7, 2024
… method (#339)

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

---------

Signed-off-by: Florian Hotze <[email protected]>
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

Successfully merging this pull request may close these issues.

4 participants