Replace Nashorn with GraalVM JS Engine

[BlasiusSecundus]

Pull Request type

• [x] Feature

Changes in this PR

Issue https://github.com/Netflix/conductor/issues/2312 replace Nashorn with GraalVM JS Engine

Nashorn is deprecated in JDK 11 and removed in JDK 15, resulting in runtime errors (and test failures) on JDK 15+

[BlasiusSecundus]

This is an updated version of my previously closed MR on the same topic (https://github.com/Netflix/conductor/pull/3233).

I have updated it to the latest version of GraalVM. Otherwise there is no substantial change.

[c4lm]

This is an updated version of my previously closed MR on the same topic (#3233).

I have updated it to the latest version of GraalVM. Otherwise there is no substantial change.

@BlasiusSecundus As much as I would personally like this change to happen, GraalJS is not fully compatible with Nashorn, and this change will break existing workflows of users. A better way would be to mark it as deprecated and do something with UI so that ECMAScript UI label corresponds to "graaljs" evaluator (and is selected as default for new workflows), at the same time we can still have old Nashorn on Java 15+ if it is included as external library from openjdk.

OpenJDK security EOL ends in 3 years Coretto in 4 years Eventually (hopefully sooner than later) Conductor will move to Boot 3 and Java 17+

[BlasiusSecundus]

Alternatively, since the script engine is selected at runtime, if Nashorn engine could not be retrieved (i. e. Conductor runs on a JDK where it is already removed, and not added manually), then automatically create GraalVM engine instead of failing. And perhaps do some logging there which engine was selected ( + update docs to reflect this change in behaviour).

[v1r3n]

@BlasiusSecundus I am trying to merge this but the builds are failing - can you run splotlessApply on the code?

[BlasiusSecundus]

I ran spotlessApply, it seems there were indeed some minor things like import ordering. Spotless check should now pass.

[v1r3n]

@BlasiusSecundus upon further testing we found that some of the scripts changes the behavior with graalvm. A good example is how maps are accessed with nashorn and graalvm.

We have two options:

1. Replace nashorn completely, add a warning for users who are updating to the later versions that they should test their workflows for graalvm compatibility before upgrading.
2. Add it behind a feature flag, we already merged the PR that adds nashorn has a library and upgrades to JDK17, so we should be able to support both. Maybe graalvm could be a another type of script?

cc: @c4lm

Other than this, the PR looks good.

[BlasiusSecundus]

If it causes backward compatibility issues, I would certainly not just replace it (as it would be essentially a breaking change).

In this case I think that the feature flag approach would be more appropriate.

[BlasiusSecundus]

As for inline task and switch operator:

evaluatorType Type of the evaluator. Supported evaluators: value-param, javascript which evaluates javascript expression.

This could be easily extended with javascript-graalvm whilst the old javascript would continue to use Nashorn

For Do-While loopCondition, Nashorn seems to be hard-coded:

Condition to be evaluated after every iteration. This is a Javascript expression, evaluated using the Nashorn engine.

Therefor introducing GraalVM there (without forcing it) could be more tricky. (Maybe a viable solution would be to introduce the evaluatorType parameter there, defaulting to Nashorn to keep the existing behavior by default)

[v1r3n]

how about we introduce a new evaluator type graaljs and if specified will use graal to evaluate the script. This will ensure existing javsascript continues to work and there is a safe path to upgrade to graalvm.

[BlasiusSecundus]

how about we introduce a new evaluator type graaljs and if specified will use graal to evaluate the script. This will ensure existing javsascript continues to work and there is a safe path to upgrade to graalvm.

Yes, @v1r3n this is what I was also intended to suggest in my last comment.

[github-actions[bot]]

This PR is stale, because it has been open for 45 days with no activity. Remove the stale label or comment, or this will be closed in 7 days.

[BlasiusSecundus]

This PR is stale, because it has been open for 45 days with no activity. Remove the stale label or comment, or this will be closed in 7 days.

Do not close.