context.evalSync() within $0.applySyncPromise() executes promise handlers in the isolate

[lev-kazakov]

Which can lead to the following unexpected behaviour:

    const isolate = new ivm.Isolate()
    const context = await isolate.createContext()

    await context.evalClosure(
        // language=JavaScript
        `this.runSyncPromise = () => $0.applySyncPromise()`,
        [() => {context.evalSync('')}],
        { arguments: { reference: true } }
    )

    // language=JavaScript
    context.eval(`
        Promise.resolve().then(() => {
            // inspecting the stack will reveal this executes in the context of the eval.
            // the expected behaviour is to have this micro task executed only after (a) this.runSyncPromise() returned.
(b)         this.runSyncPromise()
        })
(a)     this.runSyncPromise() // throws Error: This function may not be called from the default thread
    `)

[laverdet]

I agree that this is surprising.

I came up with this example which is a little easier to reason about:

const ivm = require('isolated-vm');

const isolate = new ivm.Isolate;
const context = isolate.createContextSync();

context.evalClosure(`
	let resolved = false;
	Promise.resolve().then(() => resolved = true);
	$0.applySyncPromise();
	return resolved;
`,
	[ () => context.evalSync('') ],
	{ arguments: { reference: true } },
).then(console.log).catch(console.error);

Here, the surprise is that this returns true. I think it's actually explicit in the JS spec that this should return false, but I'm not about to dive in and find out.

applySyncPromise is intended as a way to let you implement, for example, fs.readFileSync (in a child isolate) by means of fs.readFile (in the nodejs isolate) and therefore avoid blocking the main nodejs thread. It bends the rules a little bit because otherwise it would be impossible to simulate these existing APIs. Calling back into isolated-vm while the isolate is already suspended in another thread is a situation I hadn't considered and opens up a new can of worms.

My gut instinct is that this should be explicitly forbidden. The dangers of applySyncPromise can be demonstrated by changing evalSync in this example to eval. This results in a deadlocked isolate which cannot be disposed or diagnosed by any means. That's definitely not something I want to encourage.

The other option would be to maintain two microtask queues. I'm not sure how much complexity this would add, it would be something I need to play with.

[lev-kazakov]

yeah, by the spec it should have returned false, this is kinda inferred from here.

unless this is fixed, there is no way i can think of to implement the importScripts() api within the isolate.

[laverdet]

Wouldn't importScripts look something like this?

  await context.evalClosure(
    `importScripts = (...args) => $0.applySyncPromise(undefined, args).map(eval)`,
    [ async (...files) => {
      const contents = await Promise.all(files.map(file => fs.promises.readFile(file, 'utf8')));
      return new ivm.ExternalCopy(contents).copyInto({ release: true });
    } ],
    { arguments: { reference: true } },
  );

Maybe you have some pre-compiled Script objects that you're trying to reuse?

[lev-kazakov]

yeah, it'll work thx. i guess you can just return await and you don't need the return new ivm.ExternalCopy(contents).copyInto({ release: true }); right?

[laverdet]

I think you need to use ExternalCopy manually here because the return value is an array (non-transferable) and applyPromiseSync doesn't support { copy: true } because of annoying technical issues.

[lev-kazakov]

100% thx.