Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent Scripts from running indefinitely #96

Merged
merged 1 commit into from
Nov 12, 2019
Merged

Prevent Scripts from running indefinitely #96

merged 1 commit into from
Nov 12, 2019

Conversation

jerome-baudoux
Copy link

The current version of Nashorn Sandbox does not guarantee that scripts will be stopped if they consume too much memory or CPU time.

The problem occurs when the JsEvaluator takes to much time to register its thread to the ThreadMonitor

When this issue happen the following code is executed:

if (threadToMonitor == null) {
  throw new IllegalStateException("Executor thread not set after " + maxCPUTime / MILI_TO_NANO + " ms");
}

The ThreadMonitor will return an error to the user. However this won't prevent the actual script to be executed.
Even worse it will be executed without any kind of monitoring, the script can take as much time and memory it pleases him.

It would be a way better solution to not execute the script if the ThreadMonitor is not actively monitoring the script.

@mxro
Copy link
Collaborator

mxro commented Nov 12, 2019

Sorry for the late reply. Looks very good. Merging this in and will prepare a new release with this change included.

Thank you very much for this improvement!!!

@mxro mxro merged commit bb2cb8c into javadelight:master Nov 12, 2019
@mxro
Copy link
Collaborator

mxro commented Nov 12, 2019

0.1.27 including this change has been pushed to Maven Central and should be available there within the next few hours.

mxro pushed a commit that referenced this pull request Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants