Rules and concurrency

[Nadahar (Nadar)]

@rlkoshak I’ll reply to this comment here, in an attempt to move the discussion to the forum.

@rlkoshak

Yes. In fact, you can access Objects in the cache from different languages. If I put something in it at key “foo” using JS, I could pull it out in a Python script or Rules DSL. It’s only JS (as far as I’m aware) that has a restriction that the contexts need to be locked. And any changes made to the cache will impact all the automation add-ons.

I’ve found the implementation in Core, it’s called ValueCache. I can see that the cache itself is thread-safe (retrieval and storage is protected by locks), but the objects themselves aren’t (they aren’t cloned, and frankly, can’t easily be cloned), so that if one script changes the object, it will potentially change the object in the cache directly, without any concurrency protection. I don’t know the mechanism that sits between the scripting engines and this cache though, I kind of doubt that the Java objects are useful as they are. So, there probably is a conversion along the way, which would “break the link” to the actually cached object, and make it thread-safe because changing it from a script would then only change the converted object. If so, changes wouldn’t be reflected without putting it back to the cache after changing it. The only way this can be “unsafe” is if the object is used directly, as-is.

@rlkoshak

I created an issue to implement exactly that. Maintainers decided that it was a misuse of the shared cache to put Objects in it like that so they implemented the warning instead.

I’m not familiar with that discussion, but it seems to me like such a cache could be very useful. It’s probably too late to change the existing cache in any drastic way now, but an additional cache could always be added, with different rules. I was thinking about for example serializing everything to JSON in the cache, and let the individual langauge add-on transform the objects to/from something that is useful for that language. It could then become the ObjectCache

@rlkoshak

I do not, but as I said this idea has already been rejected. Based on how it’s used I suspect it’s backed by a Map of some sort. But I think it’s a Registry (like ItemRegistry).

As stated above, I found it. I’d have to see the exact discussion where it was rejected to understand exactly what was rejected, I don’t understand how the ability to share language objects wouldn’t be usefult.

ValueCache is backed by a Map<String, Object>.

@rlkoshak

Yes of course. That was the first test I did. And I have already said that I did at least twice.

Once again, I tested with two managed rules, both rules in the same .js file , and each rule in it’s own .js file.

And @florian-h05 explicitly stated how the script engine is handled for managed rules. Each script action and each script condition gets its own ScriptEngine.

I must have missed something then. I could only see one full rule quoted. But, I believe you when new script engines are created for everything.

It just didn’t make any sense to me, why make a ScriptEngineManager if you’re just going to produce new ones? But, given this explanation:

@florian-h05

The GraalJSScriptEngine is basically a wrapper around a GraalVM Polyglot Context, and the GraalVM Polyglot Engine used under the hood is already shared across all GraalJSScriptEngines. FYI, Python Scripting provides no ScriptEngine implementation, so @holger_hees simply build one around the GraalVM Polyglot Context.
And as we need separate contexts, we can also keep separate engines.

..it starts to make some sense to me. I’m speculating now of course, but what I see could have happened in that originally, ScriptEngineManager did manage script engine instances. But, because of the… “arrangement” with bridging Polyglot Engine and Context through JSR223 where a Polyglot Context becomes a ScriptEngine, this wouldn’t have worked. So, ScriptEngineManager has probably been “tweaked” into producing new engine instances, “because they are cheap in reality as they are just contexts”. “Real” JSR223 implementations will suffer of course, because they actually have to create a whole new engine each time.

Maybe this isn’t how it happened, but at least it’s a model where the pieces can start fitting together in my head.

To me, this seems like a suboptimal design, and I’m questioning if it wouldn’t be better to make something in Core that would allow scripting either through “Polylot” or “JSR223”. But, there might be lots of hurdles in the way, making it difficult.

@florian-h05

JavaScript objects can be passed to Java, GraalJS takes care of conversion.
For example a JS object consisting of simple key value pairs is converted to a Map. That behaviour is described somewhere in the GraalJS docs.

Though I don‘t think there are many complex types that end up in the shared cache, as language interoperability only really works for primitive types.
The only exception being commonly stored into the shared cache are openHAB timers. When sharing them from JS Scripting, accessing them from another thread at the same time cause a illegal multithread access requested for context … exception. Seems like there is a reference from the Java timer object to the context, at least if the timer is created through that script. But that reference might be something internal to Graal …

The details and limitations of this conversion would be of interest to the user of any particular scripting language, and should probably be documented to some extent. It still can’t understand that such conversion doesn’t mean that a new object is created, breaking the link to the “live” cached object. But, the devil might be in the details, maybe GraalVM attempts to do some kind of “live” conversion, where the converted object would reflect changes in the original and vice versa. If so, concurrency would obviously be an issue.

The fact that the timers cause exceptions indicates that they aren’t converted, but are somehow forwarded as Java objects?

I’m not so sure that it means that there’s a reference from the timer to the context, I read somewhere that GraalVM explicitly forbids objects that might cause concurrency issues, so the exception might simply be the result of this: GraalVM deduces that this object can cause concurrency issues, so it isn’t “allowed” - throw exception instead.