[jsscripting] Minor fixes & improvements

[florian-h05]

Fixes #13950.

Description

Completes a few minor improvements and fixes that were left over from #13924:

• Make sure to also unlock the ReentrantLock on exceptions from all synchronized ScriptEngine methods
• Fix the wrong implementation of createScriptEngine

For more details, please have a look at the commits (messages).

Testing

This shouldn't change the behaviour of the addon, but anyway I tested this (not extensively), works fine.

@J-N-K I've fixed the createScriptEngine implementation.
@jpg0 I made sure that all exceptions are handled and the lock is removed. I also call the super methods from their overrides.

[jpg0]

Great! You may want to extract out the exception processing into a single method, such as:

private void processAndThrow(Exception e) throws ScriptException {
        Exception processed = afterThrowsInvocation(e);

        if(processed instanceof ScriptException) {
            throw (ScriptException) processed;
        } else {
            throw new UndeclaredThrowableException(processed);
        }
}

so that you can just:

    public Object eval(Reader reader, ScriptContext scriptContext) throws ScriptException {
        try {
            beforeInvocation();
            return afterInvocation(super.eval(reader, scriptContext));
        } catch (Exception e) {
            processAndThrow(e);
        }
    }

And not have to repeat the exception logic for each method.

Either way I'm happy though.

[florian-h05]

FYI I‘ve switched from the e instanceof XXXException checks to catch (XXXException e) since SAT showed warnings. I can try out what SAT does when I introduce a exception processing method.

@jpg0 Although I'd prefer to extract that, it's not possible because we are then missing the return statement and therefore get a compilation failure: [61,19] This method must return a result of type java.lang.Object.

As your are also happy with the current state, let's keep and get this merged.

[florian-h05]

@jlaur Can you please merge this? @jpg0 is happy with the changes.

[jpg0]

@jpg0 Although I'd prefer to extract that, it's not possible because we are then missing the return statement and therefore get a compilation failure: [61,19] This method must return a result of type java.lang.Object.

One option is to throw an InternalError or AssertionError but it's debatable whether that's better style, happy to keep as-is.

(It's awkward because the throws clause in a Java method allows declaring an implicit union-type of exception classes via multiple exception types, but the language itself doesn't actually support union types, making it impossible to return rather than throw the same set of exception types.)

[florian-h05]

@kaikreuzer Please consider to merge this before the release candidate is published and we have a code freeze.

[jlaur]

@florian-h05 - we have a code freeze now. However, perhaps you can help me with some clarifications, because it's not obvious to me which concrete issue is resolved with this PR. From the linked issue two issues are mentioned:

@J-N-K commented in #13924 (comment) on GraalJSScriptEngineFactory:
In general this implementation is wrong, you should check whether the requested scriptType is supported by the factory and return null if it is not. However, that is beyond the scope here, just noticed it.

The way I read this, is that it's a design flaw, but the implementation in current state will work (for now), so it's more like a technical improvement?

@jpg0 commented in #13924 (comment) on InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable:
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.

I also read this as a matter which is not urgent to fix?

So my conclusion is that #13924 already contained the "MVP" fixes needed for the release, while the mentioned two mentioned left-overs were intentionally left to be fixed later, because we wanted to merge the PR without further delay.

Therefore, and please correct me if I'm wrong, it seems we should not risk merging this PR before the release, as it could cause regressions which we have no time to discover and fix. Cc @jpg0, @J-N-K, @kaikreuzer

[kaikreuzer]

I agree with @jlaur - we can only merge really critical fixes now and unless we are talking about something severely broken right now, we should not take the risk of a merge.

[florian-h05]

@jlaur

The way I read this, is that it's a design flaw, but the implementation in current state will work (for now), so it's more like a technical improvement?

You are absolutely right, it‘s a design flaw but in fact it is working fine for 1 year on my system, so it‘s only a technical improvement.

I also read this as a matter which is not urgent to fix?

You are again right. This is again more a technical improvement, because ScriptEngine.eval should not throw anything other than a ScriptException and this one is already covered.
I guess if we got any other exception here, we have other problems with JS Scripting than a deadlocked script.

My intention behind this PR was to get these technical improvements into the RC1 to battle-test them, but I now totally agree with both of you (also @kaikreuzer) that we shouldn‘t risk a merge now. So let‘s wait for the release and then merge after it.

The current implementation works fine, this PR only improves it a bit from the technical side.

[J-N-K]

This is correct. The factory should not be called with an incompatible script type (that would be a bug in core) but if that was the case (which is not ATM), then it should return null and not a script engine that can't handle the requested script type.

[florian-h05]

@kaikreuzer Documentation & translations updates shouldn‘t be a problem, correct?

[kaikreuzer]

@kaikreuzer Documentation & translations updates shouldn‘t be a problem, correct?

Yes, that should be fine.

[florian-h05]

@jlaur
Since the stable is released I’d guess the code freeze is over and we can merge this?

[jlaur]

@jpg0 - can you do a final review of this PR and approve if it looks good to you?

[jpg0]

A single minor issue, other than that it lgtm

[jpg0]

@jlaur I've submitted my review, however I don't have access to approve/merge.

[florian-h05]

@jpg0 Done, see 9289764.
@jlaur I addressed the review.

[florian-h05]

@kaikreuzer I think @jpg0 is missing in the contributors group and therefore can't review PRs and also is not automatically requested for review here although he is a codeowner.

Can you please add him to the contributors group?

[jlaur]

Thank you!

[florian-h05]

The build is failing, seems like the missing openHAB 4.0.0 artifactes are the problem.
My local build is working fine.

[jlaur]

@florian-h05 - yes, we might need the nightly core build first. This is not the only PR having this issue.

[florian-h05]

@jlaur All builds except the Java 11 build succeeded. The Java 11 build has been removed a few minutes ago, I think we can therefore ignore its failure.