javadelight / delight-nashorn-sandbox

A sandbox for executing JavaScript with Nashorn in Java.
Other
263 stars 81 forks source link

Multi-thread environment question #77

Open sinichkin opened 5 years ago

sinichkin commented 5 years ago

Hi,

In my program I evaluate JavaScript snippets parsed from html pages. My aim is to detect JavaScript redirection. My question is: should I create sandbox for every thread? NashornSandbox sandbox = NashornSandboxes.create();

In case I can use the same sandbox with different contexts which executor should I use in case I want to limit CPU usage? sandbox.setExecutor(?); I believe single thred executor cannot be used for multi-threaded environment.

mxro commented 5 years ago

Hi There,

Thank you for raising this issue.

Is there anything preventing you from creating a sandbox for every thread? If not, creating a sandbox for every thread would probably be the safest thing to do!

martin-1120 commented 4 years ago

create sandbox is too expensive, has any way to reduce sandbox.eval(script,bind) cost time?

junweilivnera commented 1 year ago

@mxro I have similar question about reusing NashornSandbox instance in multi-thread env. If I create a new NashornSandbox instance for every single thread, I notice that too many jdk.nashorn.internal.runtime.ScriptLoader are created, which lead to metaspace oom. So is it safe to use a threadpool with size more than 1 in setExecutor()? In delight-nashorn-sandbox code repo, all the tests are using Executors.newSingleThreadExecutor(), hence asking. Thanks.

mxro commented 1 year ago

@junweilivnera Thank you for your question!

In general, I think it will probably be good to keep executions for one sandbox instance to one thread, since a sandbox only has one script context, see https://stackoverflow.com/questions/34165606/nashorn-in-thread-safe-manner

However, a possible optimisation could be to only ever create one script engine and pass that when creating the sandbox? The constructor on the class supports that (but not exposed through NashornSandboxes.create())

https://github.com/javadelight/delight-nashorn-sandbox/blob/master/src/main/java/delight/nashornsandbox/internal/NashornSandboxImpl.java#L95

coldgust commented 1 month ago

Hi @mxro , It seems will lead to sandboxClassFilter invalid if use the constructor with ScriptEngine params. Maybe need to add a constructor with ScriptEngine and SandboxClassFilter params.

mxro commented 1 week ago

@coldgust Did you try creating it via:

new NashornSandboxImpl(scriptEngine);

I think that should work?