Changes to instructionLimit and MaxDuration is not thread safe

[gtskaushik]

Using a singleton RhinoSandbox object

1. Thread1 or the first invocation of eval method creates a globalContextFactory. The instance variable in RhinoSandboxImpl class and the globalContextFactory are one and the same.
2. Any mutations to contextFactory like instructionLimit and MaxDuration will change the globalContextFactory as well
3. In eval method, we get a new context by calling SafeContext.makeContext which is always using the globalContextFactory rather than using the currentFactory.
4. Because of point 3, all mutations affect globalContextFactory. So, unless we put a synchronized on the method which calls RhinoSandboxImpl.eval, we are not thread safe

Using a RhinoSandbox object per Thread (RhinoSandbox is kept in a ThreadLocal). This is to mitigate the race condition problem

1. Thread1 or whichever thread is first, it creates a globalContextFactory. The instance variable in this RhinoSandboxImpl object and the globalContextFactory are one and the same.
2. Thread2 calls eval. Now, globalContextFactory is already set. Now we get a new context by calling SafeContext.makeContext which is always using the globalContextFactory rather than using the currentFactory.
3. Because of point 2, the mutations to instructionLimit and MaxDuration are not reflected during eval execution.

Test Case

public class ScriptEngineServiceTest {
  @Test
  public void multiThreadingFail() throws InterruptedException {
    CountDownLatch latch = new CountDownLatch(2);
    Thread t1 = new Thread(() -> {
      var rhinoSandbox = RhinoSandboxes.create();
      String jsCode = "(function a() {return \"test\";})()";
      rhinoSandbox.setMaxDuration(0);
      rhinoSandbox.setInstructionLimit(0);
      rhinoSandbox.eval(null, jsCode);
      latch.countDown();
    });
    t1.setName("Thread 1");

    Thread t2 = new Thread(() -> {
      var rhinoSandbox = RhinoSandboxes.create();
      String jsCode = "(function a() {\n" + "    while(true) {\n" + "    }\n" + "})()";
      rhinoSandbox.setMaxDuration(500);
      rhinoSandbox.setInstructionLimit(0);
      rhinoSandbox.eval(null, jsCode);
      latch.countDown();
    });
    t2.setName("Thread 2");

    t1.start();
    t2.start();

    latch.await();
  }
}

The above code is expected to fail with ScriptDuration Exception but doesn't. But if we start Thread 2 first, we get the expected exception

Proposal

SafeContext.makeContext should use org.mozilla.javascript.Context(ContextFactory factory) instead of org.mozilla.javascript.Context(). This is also recommended by the Java docs in org.mozilla.javascript.Context()

[gtskaushik]

@mxro If the proposed fix is fine, I can create a PR for the same

[mxro]

@gtskaushik Thank you for reporting this issue and documenting the issue very well!

Yes I think this will be a great way forward to make the library more thread safe. Please be welcome to prepare a PR 👍

[gtskaushik]

@mxro Created a PR - https://github.com/javadelight/delight-rhino-sandbox/pull/19

[mxro]

Can you confirm this is now all fixed and close the issue if that is the case? Thank you!

[gtskaushik]

It is working now. Closing the issue