Support for GraalJS engine #85 #87
Conversation
There was a problem hiding this comment.
Hello, I have taken an interest in the Graal implementation of this. I had a review of the changes and I am worried about some of the changes to the handling of securing bindings. Perhaps there are some differences in the way in which GraalJS handles bindings, so if you could clarify your reasoning behind certain changes here it would be appreciated.
| return new GraalSandboxImpl(); | ||
| } | ||
|
|
||
| public static NashornSandbox create(String... args) { |
There was a problem hiding this comment.
Could this file include equivalent javadocs?
| } | ||
|
|
||
| @Override | ||
| protected Bindings secureBindings(Bindings bindings) { |
There was a problem hiding this comment.
This method appears to be doing the opposite of what the other is doing, adding everything it sees to the cached bindings.
I am not sure any of these methods need to be overridden as the bindings behaviour seems much the same. These look like they are trying to do a cached bindings version instead, in which case a non-sandboxed implementation would be better.
There was a problem hiding this comment.
Currently we can only use one set of bindings in GraalJS due to the limitations that objects cannot be shared across bindings/contexts (except for primitives). Once Graal adds support for this we can remove these overrides.
There was a problem hiding this comment.
Just to confirm the behaviour here then,
If a binding for "a" -> "b" is passed in to eval, will that persist between each call of eval?
If it does that is not really desired since that can lead to a leak from one script to another.
There was a problem hiding this comment.
You're right - I fixed the bindings leak from one script to another. Thanks!
|
|
||
| @Override | ||
| protected void resetEngineBindings() { | ||
|
|
There was a problem hiding this comment.
This again does not allow for bindings to be reset which is a problem for secured bindings.
There was a problem hiding this comment.
oracle/graal#631, oracle/graaljs#47 and oracle/graaljs#146 prevent resetting of bindings for GraalJS the way NashornSadbox does it for the Nashorn engine. You'd run into Exception like these: org.graalvm.polyglot.PolyglotException: java.lang.IllegalArgumentException: Values cannot be passed from one context to another. The current value originates from context 0x7ba8c737 and the argument originates from context 0x74294adb.
src/main/java/delight/nashornsandbox/internal/NashornSandboxImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/delight/nashornsandbox/internal/NashornSandboxImpl.java
Outdated
Show resolved
Hide resolved
| if (engineBindings.get(key) != null) contextBindings.remove(key); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
What do these lines aim to do, and if they meet a need why not have it in the other method?
There was a problem hiding this comment.
If a script context is provided, its bindings will be evaluated later - i.e. inside the script engine - and merged into the engine bindings. Nashorn automatically checks against global bindings but Graal seems to override global binding entries if the same key is present in the script context's bindings. This will prevent overriding globals. We don't know if this is intentional behavior by GraalJS.
|
@everestbt Thanks for the feedback - I'll take a more detailed look later, address specific comments and apply suggested changes. The main reason for the "strange" overriding behavior is due to oracle/graal#631 and oracle/graaljs#146 which makes sharing objects across contexts (every Bindings object is a new context) impossible. Thus we have to merge passed additional (simple) bindings into the current engine bindings. If and when GraalJS allows sharing objects across multiple context we can simply remove the current overrides and everything should work. |
|
@everestbt package private changes and comments have been applied. |
|
@mellster2012 Thank you for the clarification, the choices made make a lot of sense and when/if they are brought closer it allows for very easy combining of the methods. I have just posted one further question on the leak of bindings, thank you. |
|
@everestbt The bindings leak has been fixed. Pls. review and let me know if you have any other concerns - thx! |
|
@mxro @everestbt any comments or updates? Thx. |
|
Looking all good to me! Thank you @mellster2012 . @everestbt All good to be merged from your view? |
|
@mxro @mellster2012 All looks good from my angle. I have not had a chance to mess around with it too much but should be able to shortly. I think it is a great idea to merge it. May be worth noting on the release that the GraalJS implementation is initial. |
|
@mxro @everestbt sounds good - Max, can you merge the conflicting test and add a comment to the release that the GraalJS implementation is initial? Thanks! |
|
Merged and released. Thank you! |
|
I have published the merged version to maven central under the version After this, I did some refactoring work and moved the Graal Js related code into a new module: https://github.com/javadelight/delight-graaljs-sandbox This module has a dependency on Nashorn Sandbox and reuses the NashornSandboxImpl class from there. I did this because of the following:
I have set up a CI pipeline for the graaljs sandbox and published the module on BinTray. A request to have it included in Maven central is currently pending. I think ideally we could have a third package 'js-sandbox' from which both Nashorn Sandbox and GraalJs sandbox import. But I think that may be a project for another day. |
|
Perfect - I was going to suggest module separation as next step to avoid unnecessary dependencies. Thanks! |
Hi Max,
This PR makes minimal changes to the NashornSandboxImpl, most modified files are test files, adding a GraalJS test for each test while leaving the original tests unmodified. The GraalSandboxImpl inherits most features from the NashornSandboxImpl with the exception of a few overrides. It's not necessarily the cleanest solution and we can keep working with our internal fork while migrating to GraalVM, but creating and maintaining a fully separate fork/project would either require a lot of base refactoring for both projects or lead to a lot of duplicated code for the GraalJS version. The engines are similar enough in behavior that we could probably make just few backwards-compatible continuous changes (gearing it more towards GraalJS) while Oracle is phasing out Nashorn (which is static at this point) gradually and starts shipping GraalJS. Take a look and let me know what you think, feel free to make edits.
cheers