I haven’t read through it all yet, but I already see some of the things I’ve been saying being mentioned:
IMO that’s wrong usage of the cache. You should store unmodifiable objects there, and instead of modifying an existing object create a modified copy and put that to the cache.
The problem is that people can all have their ideas about how something should be used, if it isn’t enforced, or at least strongly “recommended”, that won’t be what happens.
Further:
I think the issue really is that you try to use an object that itself does not support multi-thread access. It’s not an issue of the cache. Try something simple, put an integer to the cache, start with 0, increase after every read and put the new number to the cache. This should work perfectly fine.
Again, the same idea, as I’ve said, this cache is only safe under two circumstances: Immutable objects and thread-safe objects. But this isn’t enforced, or even properly warned about, so people will put mutable objects there. There is no way that can be done safely, and I think GraalJS is right to refuse to do it, instead of just going along with the “russian roulette”.
This might indicate what I’d call a “serious blunder”, if this is actually recommended/documented:
Note that my original repro is based on an example from the documentation, which uses an object with a property that serves as a counter. This suggests that this is a totally normal thing to do and should not result in exceptions. See here: JavaScript Scripting - Automation | openHAB
You can’t use a shared cache without locking for “counting logic” even with primitives, it will never do what you expect. You’d need to use something like AtomicInteger to do that with consistency, simply because two threads can be updating the counter at the same time, and one will “overwrite” the increment done by the other.
This is actually an interesting case, because it has a built-in solution that nobody seems to use or know how to use:
I have a case where the only reason I’m using the shared cache is so the rule condition and action can share some data. It can never be the case that both will run at the same time because they are part of the same rule. Can the warning log statement be made smarter to detect a case like this where the shared cache is being used in a way that is safe even when using JS Objects? I think all it would take is looking to see which rules reference the entry and if it’s only one rule it’s not a problem. I don’t know if that info is available though.
There are inputs and outputs to/from modules (triggers, conditions, actions), that are meant to do exactly this - pass data between them. I don’t know exactly how to use it either, but I doubt that it’s rocket science. The question is how many things have been done to break this in the meanwhile, since people haven’t been aware that it’s there.
Regarding the issue (Using cache.shared in JavaScript rules can lead to unexpected java.lang.IllegalStateException: Multi threaded access requested by thread #4413) as a whole, I’d say that neither 1), 2) or 3) are viable. You simply can’t rely on GC, or any other “implied mechanism” to handle the locking and unlocking for you, it will be way too slow and lock the objects for far too long.
I do see one way this particular functionality could be done, but it with bring with the risk that users could cause deadlocks by mishandling the locking in their scripts: You could create a (3rd ?) shared cache with dedicated locking mechanisms, where you’d either have to “lock the key” before you could retrieve its value, or where retrieving it automatically locked it. But, the cache would then also have to have an unlock(<key>) method that you had to call using “try.. finally”, or a corresponding mechanism in the language in question, as soon as you were done dealing with that object.
It would allow doing “shared counters” and other things like that, but it would fail spectacularly when users didn’t understand the locking/unlocking, and I can’t see that it’s very easy to “warn” if they do it wrong. But, it’s the only way I can see that you could actually achieve what was asked for in a thread-safe, not “by luck”, manner.