javadelight / delight-graaljs-sandbox

A sandbox for executing JavaScript with Graal in Java
Other
44 stars 11 forks source link

Promises Issue with GraalJS Sandbox #5

Open binaryoverload opened 3 years ago

binaryoverload commented 3 years ago

Hi there,

We're trying to run a script inside the sandbox that utilises promises. However, it seems that promises do not work?

When running code num 1, the message args [] is printed out (A successful result). However, when running with code num 2, nothing is printed.

Code Num. 1

(async () => {
  const c = channel.sendMessage(`1`);
  println.accept(c instanceof Promise); // => false
  const msg = await c;
  channel.sendMessage(`2`); // => does run
)()

Code Num. 2

(async () => {
  const c = new Promise(channel.sendMessage(`1`));
  println.accept(c instanceof Promise); // => true
  const msg = await c;
  channel.sendMessage(`2`); // => doesnt run
)()

Do you have any ideas about why this might happen? We have run the same code using just the GraalJS script engine and it works fine!

Any help would be appreciated 😃

cc: @mrjvs @weeryan17

mxro commented 3 years ago

Thank you for submitting this issue!

Is there anything you can see in the generated code? Maybe something goes wrong with the obfuscation?

Here the config option that should print the generated code:

log4j.logger.delight.graaljssandbox.internal.GraalSandboxImpl=DEBUG
weeryan17 commented 3 years ago

We found the issue. When the sandbox finishes running you remove any added bindings which happened to include our promises as they where returned from objects passed in via bindings in the eval method. https://github.com/javadelight/delight-graaljs-sandbox/blob/7b57f6fe93040548db59f427642a1bf3750500ea/src/main/java/delight/graaljssandbox/internal/GraalSandboxImpl.java#L172 The main problem is the code technically finishes running as the main thread has finished, but promises are run on a separate thread.

Edit: I should note we know that this is for sure the issue as we removed just that line in our own fork and it fixed the problem. Not the best solution, but it worked.

mxro commented 3 years ago

Thanks for getting back - that is good to know. Any idea how this could be modified to work safely for this usecase out of the box or if some kind of warning could be issued?

weeryan17 commented 3 years ago

The best solution I can think of right now is just to have an option along the lines of removeAddedBindings, and let people set it to false with the default being true (to not break the current behaviour), or have that option per binding.