[jsscripting] Extend synchronization to common ScriptEngine methods

[florian-h05]

Description

This extends the multi-thread synchronization to "eval" and "invokeMethod" and moves synchronization for "invokeFunction" to the InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable class.
The synchronization mechanism changed from using synchronized to using a ReentrantLock together with catch_finally to avoid having deadlocks when an exception is thrown.
Fixes the multi-thread access requested warnings described in the community (https://community.openhab.org/t/openhab-3-4-milestone-discussion/138093/130) and related to openhab/openhab-core#3180.

Testing

I've tested this (see https://community.openhab.org/t/openhab-3-4-milestone-discussion/138093/150), I've also verified on my production system that this does not cause any issues with rules and timers.

/cc @ccutrer @cweitkamp @J-N-K

The pull request should be built automatically and available at:
https://openhab.jfrog.io/artifactory/libs-pullrequest-local/org/openhab/addons/bundles/org.openhab.automation.jsscripting/3.4.0-SNAPSHOT/org.openhab.automation.jsscripting-3.4.0-SNAPSHOT.jar

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/openhab-3-4-milestone-discussion/138093/150

[ccutrer]

Seem simple enough to me. Move the lock up to a parent class, and have it cover more entry points. I'm not familiar enough with the the entire hierarchy of script engines here though, so this is a weak approval at best.

[florian-h05]

@jpg0 Can you please have a look at this? This is urgent since the code freeze will come on Friday evening.

I know that you might not like the synchronization inside DelegatingScriptEngineWithInvocableAndAutocloseable, but for me this seemed to be the best place, because:

• InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable was no option to me since it is there for

  interception of calls, either before Invocation, or upon a ScriptException being thrown.

• OpenhabGraalJSScriptEngine: the place where you wanted me to put the synchronization for invokeFunction, but adding the overrides with the synchronization logic here makes it unnecessarily “puffed up” IMO.
• DelegatingScriptEngineWithInvocableAndAutocloseable is there to Allow overriding of specific methods., which is exactly what we need here.

[jpg0]

You are correct that DelegatingScriptEngineWithInvocableAndAutocloseable is there to allow overriding of methods. But it is not there to add specific functionality itself, rather to make it easy for other classes to do so.

The way these delegates work is my providing the 'template' for implementors to add the functionality. Their responsibility is providing 'hooks' for implementors to use. In this case one option would be to subclass the delegate and add your specific functionality to the subclass. Another would be to create an additional delegate type which wraps this delegate - you could even do this with a dynamic proxy if you just wanted to synchronise every method.

The reason that I am against adding it to the delegate itself is that the purpose of the delegate is to create a separation from target class (the script engine) to specific behaviour you want added in other classes. If you start putting the specific behaviour in the delegate itself, then there is no longer any separation anyway, so the purpose of the class existing is lost.

Also if you look in the openhab-core codebase you will see many DelegatingXXX classes. These all behave in exactly the same way, because a delegate is a design pattern that works this way and this makes it easy for developers to immediately understand what the class does.

[florian-h05]

@jpg0 So you are proposing to add another delegate class?
What do you think is the “best” approach of the three you described?

[jpg0]

Looking at it again, the best way to do this is to subclass the InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable - this is already designed to intercept all calls. You would need to add an afterInvocation to release the lock though (I never did this as I didn't have a need for it, but it's a common pattern).

The only problem is that you would need to switch to explicit use of a ReentrantLock rather than use of the synchronized keyword. TBH this is probably not a bad idea anyway as we are already going fairly deep on synchronisation here and this will give you more options.

[jpg0]

(Oh and if the previous suggestion is too complex for you, I'd also support just subclassing DelegatingScriptEngineWithInvocableAndAutocloseable and adding the synchronisation in the subclass.)

[florian-h05]

You would need to add an afterInvocation to release the lock though

Also to OpenhabGraalJSScriptEngine?
So lock in beforeInvocation and unlock in afterInvocation then?

I’ve also seen that in InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable, beforeInvocation and the ScriptEngine method (e.g. eval) is called inside try catch, afterInvocation should best be called after the try catch, otherwise a failed execution would cause a deadlock, correct?

IMO, if I implement unlocking in afterInvocation in OpenhabGraalJSScriptEngine, I'd just need to add the afterInvocation method to InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable without subclassing it.

[jpg0]

Also to OpenhabGraalJSScriptEngine?

No, this doesn't need it, unless you want to put the synchronisation into it directly (and then wouldn't need a subclass).

So lock in beforeInvocation and unlock in afterInvocation then?

Yes. You don't need to call afterInvocation after the catch, as long as you also release the lock in afterThrowsInvocation. It's more typical this way (so that afterInvocation is not called upon exception).

[florian-h05]

I think I will put the synchronizatuon logic directly into the OpenhabGraalJSScriptEngine since until now synchronization was also provided inside it.

You don't need to call afterInvocation after the catch, as long as you also release the lock in afterThrowsInvocation. It's more typical this way (so that afterInvocation is not called upon exception).

Don't I need to call afterInvocation in the finally block to unlock the lock if the try succeeds and if it fails?

[jpg0]

Don't I need to call afterInvocation in the finally block to unlock the lock if the try succeeds and if it fails?

You need to lock in before, then unlock in both after and afterThrows (only one should get called).

[florian-h05]

@jpg0 Can you please have a look at my current implementation? See 3e8bbda.

@J-N-K Can you please have a look at my null annotations changes/compiler warnings reducal? See e9708bb.

[jpg0]

You have put afterInvocation in a finally block meaning that it's called even when an exception is thrown. This is non -standard for interception code like this, plus prevents consumers hooking non-exception invocations only. Better is to keep inside the try block so that it's not called upon exception and ensure that the implementor also overrides afterException to release the lock in this case.

The locking code does lgtm though and will work.

[florian-h05]

Putting afterInvocation inside the try block wouldn’t work (if I’m wrong please correct me), since it would need to come after the return statement:

    @Override
    public Object eval(String s, ScriptContext scriptContext) throws ScriptException {
        try {
            beforeInvocation();
             return super.eval(s, scriptContext);
            // afterInvocation wouldn’t work here
         } catch (ScriptException se) {
             throw afterThrowsInvocation(se);
         } finally { // Make sure that Lock is unlocked regardless of an exception is thrown or not to avoid deadlocks
             afterInvocation();
         }
     }

The only possible option is to store the result of eval in a local var, then call afterInvocation and then return the local var. I’m not sure if that would change the behaviour of the script engine or not, I’m no ScriptEngine expert.

[jpg0]

I would do it this way:

@Override
    public Object eval(String s, ScriptContext scriptContext) throws ScriptException {
        try {
            beforeInvocation();
            return afterInvocation(super.eval(s, scriptContext));
         } catch (ScriptException se) {
             throw afterThrowsInvocation(se);
         }
     }

Then you get the added benefit that subclasses can modify or wrap the return value if they wish. (afterInvocation would accept and return an Object, and may require casting in some cases).

You can also use a local variable if you like; it shouldn't affect the delegate, regardless of whether it's a script engine or anything else.

[florian-h05]

@jpg0 Can you please have a look at my commit 7b7e01f?

From my side this only needs some testing and then can be merged.

[J-N-K]

Just one comment: According to https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/locks/Lock.html the pattern for locks should be

lock.lock();
try {
    // do something
} finally {
    lock.unlock();
}

while you use

try {
    lock.lock();
    // do something
} finally {
    lock.unlock();
}

Since we usually use the first, it would be preferable to use that.

[jpg0]

@jpg0 Can you please have a look at my commit 7b7e01f?

Looks good, only change would be to remove the left-over parameterless afterInvocation.

[florian-h05]

@J-N-K I‘ve addressed your review comment (not pushed yet).

But I found a deadlock due to a NoSuchMethodException being thrown. I already have a fix, but I have to keep afterInvocation since it universally allows us to unlock the lock without passing or returning an argument, e.g. in case we need to handle an exception that is no ScriptException. @jpg0 So I’ll keep the parameter-less afterInvocation.

I will try to finish this this evening, would be nice if we could get this merged before the next SNAPSHOT.

@jlaur Are you available?

[jpg0]

So I’ll keep the parameter-less afterInvocation.

Don't do this, just make sure that the catch is wide enough that everything is caught: catch Throwable.

[jlaur]

@florian-h05 - I'll be available in the evening, if you need an addon maintainer for merging. However, I don't know how much time I will have for reviewing this and also don't feel comfortable in this area, so I would suggest approval from @jpg0 and/or @J-N-K first. @kaikreuzer, keeping you in the loop also as this seems like an important fix, but probably also with some risk.

[florian-h05]

Commit e17f869 resolves:

• [jsscripting] Extend synchronization to common ScriptEngine methods #13924 (review)
• [jsscripting] Extend synchronization to common ScriptEngine methods #13924 (comment)
• [jsscripting] Extend synchronization to common ScriptEngine methods #13924 (comment)

Commit 87af8e7 fixes the deadlock I was talking about in #13924 (comment).

I have tested this on my dev system and also on my production system (with about 70 JS rules using timers and multiple triggers), works fine 👍

@J-N-K Can you please review?
@jpg0 feel free to also have a look, but your comment about the parameter-less afterInvocation is addressed (I removed it as asked) and the other changes are minimal.

[J-N-K]

As @jpg0 mentioned: if you catch Throwable we are sure that the lock is always removed. (Also below) Otherwise LGTM.

[florian-h05]

And then rethrow it? The method expects a return statement, so I either need to return something or throw something. If I want to rethrow it, I need to add throws Throwable.

[florian-h05]

@J-N-K
I've fixed this in 2506015, but since I cannot throw a general Exception from these methods since that would clash with Invocable.invokeMethod and Invoke.invokeFunction, I handle all exceptions (except NPEs because SAT fails then) that can be thrown from these two methods.

And, I always need to return a value.

[J-N-K]

LGTM

[florian-h05]

@J-N-K
Can you please then add an approving review so we can ping @jlaur for merging?

I think we don't need to wait for @jpg0 since his concerns were addressed and the other changes are exception handling which is nothing GraalJSScriptEngine specific.

[florian-h05]

@jpg0
See 8a6834d, I hope this is the final commit ;-)

[jpg0]

Almost :-/

You updated the exception type, but didn't make the switch from afterInvocation to afterThrowsInvocation.

[florian-h05]

@jpg0
See 3140f02.
I think I linked the wrong commit.

Maybe just have a look at the full change: https://github.com/openhab/openhab-addons/pull/13924/files.

[jpg0]

Ah yes, I see now. Great! There are two other issues*, but maybe we should look at them another time; I don't want to hold this up any longer and they are not things that are hard to change later.

• the are other methods which just cast the return of afterThrowsInvocationto ScriptException rather than performing the type checks. These casts may fail. Best is probably to extract the type checks into a separate method which each of these methods use to extract the correct exception type to throw.
• ideally the overriden implementation of afterThrowsInvocation should call super.afterThrowsInvocation(e) to get the exception to return, rather than using e directly, in case the parent class is also doing something with it.

Either way, let's get this merged!

[florian-h05]

I‘ve created issue #13950 as a reminder.

[J-N-K]

Either that, or catch Exception, I guess we‘ll have other sorts of issues if we run into an Error.

[florian-h05]

@jlaur This is ready for merging now.

See #13924 (comment) and #13924 (comment).

Please have a look at commit df17c75, it should be fine.

[florian-h05]

@jlaur Let‘s get this merged!
See the approval of J-N-K and jpg0‘s comment in #13924 (comment).

[jlaur]

Thanks!