Skip to content
This repository has been archived by the owner on Dec 13, 2023. It is now read-only.

Replace Nashorn with GraalVM JS Engine #3717

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

BlasiusSecundus
Copy link

Pull Request type

  • Feature

Changes in this PR

Issue #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
Copy link
Author

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 BlasiusSecundus changed the title feat(#2312): replace Nashorn with GraalVM JS Engine Replace Nashorn with GraalVM JS Engine Aug 3, 2023
@c4lm
Copy link
Contributor

c4lm commented Aug 10, 2023

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
Copy link
Author

BlasiusSecundus commented Aug 11, 2023

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
Copy link
Contributor

v1r3n commented Sep 11, 2023

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

@BlasiusSecundus BlasiusSecundus force-pushed the feature/2312-replace-nashorn-engine-with-graalvm branch from bad5606 to 55da066 Compare September 11, 2023 19:28
this is required for JDK 15+ compatibility
(Nashorn was removed in JDK 15)
@BlasiusSecundus BlasiusSecundus force-pushed the feature/2312-replace-nashorn-engine-with-graalvm branch from 55da066 to 6447acd Compare September 11, 2023 19:32
@BlasiusSecundus
Copy link
Author

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

@v1r3n
Copy link
Contributor

v1r3n commented Sep 16, 2023

@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
Copy link
Author

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
Copy link
Author

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
Copy link
Contributor

v1r3n commented Sep 29, 2023

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
Copy link
Author

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.

Copy link
Contributor

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.

@github-actions github-actions bot added the Stale label Nov 15, 2023
@BlasiusSecundus
Copy link
Author

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.

@github-actions github-actions bot removed the Stale label Nov 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants