More determenistic behavior of detached promises

[davojan]

TL/DR

Current behavior of promises detached from the main "thread" of function call (via Reference.apply) inside isolate causes two very non-intuitive problems:

1. Reference.apply call actually exceed the given timeout - detached promises callbacks continue to work without limitations.
2. Unhandled promise rejections that hasn't been failed fast enough cause failures of totally unrelated subsequent calls to the isolate functions with confusing stack-traces that doesn't make any sense.

Possible solution: Introduce option like waitForDetachedPromises: true and doesn't return a result until they all are resolved or rejected and kill them all if timeout is exceeded. Unfortunately I'm not aware of V8 internals and if this solution is implementable. But I think current behavior seems buggy and the suggested flag (if possible) should be enabled by default (may be in v5, as it's breaking change).

Reproducible example

Here is an example runnable with ts-node, illustrating the issue.

import { Isolate } from 'isolated-vm'

main().catch(err => console.error(err))

async function main(): Promise<void> {
  const isolate = new Isolate({ memoryLimit: 128 })
  const context = await isolate.createContext()

  // necessary functions init
  const jail = context.global
  await jail.set('global', jail.derefInto())

  await jail.set('log', function (...args: any[]) {
    console.log(...args)
  })

  await context.evalClosure(
    `
      global.timeoutPromise = function timeoutPromise(ms) {
        return $0.apply(undefined, [ms], { arguments: { copy: true }, result: { copy: true, promise: true } })
      }
    `,
    [timeoutPromise],
    { arguments: { reference: true } },
  )

  await context.eval(`
    global.handleRequestInIsolate = async function handleRequestInIsolate(detachedSucceedOrFail, delay) {
      log('handleRequestInIsolate(', detachedSucceedOrFail, delay, ')')
      new Promise(async (resolve, reject) => {
        await timeoutPromise(delay)
        log(\`Inside detached promise after \${delay}ms delay\`)
        if (detachedSucceedOrFail === 'succeed') {
          resolve()
        } else {
          reject(new Error(\`Detached promise failed in isolate after \${delay}ms\`))
        }
      })
      return delay
    }
  `)

  const handleRequestRef = await jail.get('handleRequestInIsolate', { reference: true })

  async function handleRequestClient(detachedSucceedOrFail: 'succeed' | 'fail', delay: number): Promise<void> {
    try {
      const result = await handleRequestRef.apply(null, [detachedSucceedOrFail, delay], {
        timeout: 1000,
        arguments: { copy: true },
        result: { copy: true, promise: true },
      })
      console.info(`handleRequest(${detachedSucceedOrFail}, ${delay}) succeeded with result: ${result}`)
    } catch (err) {
      console.error(`handleRequest(${detachedSucceedOrFail}, ${delay}) failed with error`, err)
    }
  }

  //------------- THE TEST ITSELF ----------------

  // this call will cause failure of the random unlucky call (third call below) after 2 seconds (!!!)
  await handleRequestClient('fail', 2000)
  await timeoutPromise(1000)

  // this call is lucky because detached promise from the call above is not yet failed
  await handleRequestClient('succeed', 500)
  await timeoutPromise(1000)

  // this one will fail because of failed detached promise created in the first call above
  // I spent half a day to figure out the issue in the similar situation in real-life app
  await handleRequestClient('succeed', 2000)
}

function timeoutPromise(ms: number): Promise<void> {
  return new Promise(resolve => setTimeout(resolve, ms))
}

Here is the output of the program (with my comments):

handleRequestInIsolate( fail 2000 )
# this shouldn't succeed, it should wait for detached promise and fail after ONE second due to apply timeout
handleRequest(fail, 2000) succeeded with result: 2000
handleRequestInIsolate( succeed 500 )
# this is the only call that should succeed, because 500 < 1000 timeout
handleRequest(succeed, 500) succeeded with result: 500
Inside detached promise after 500ms delay
# this message should never appear due to apply timeout
Inside detached promise after 2000ms delay
handleRequestInIsolate( succeed 2000 )
# here is the main problem - we've got the stack trace totally unrelated to the current call
# this call must fail though, but due to apply timeout
handleRequest(succeed, 2000) failed with error Error: Detached promise failed in isolate after 2000ms
    at <isolated-vm>:10:18
    at (<isolated-vm boundary>)
    at handleRequestClient (isolated-example.js:47:51)
    at main (isolated-example.js:74:11)
# this message also should never appear due to timeout
Inside detached promise after 2000ms delay

Side note about unhandled rejections

Given the above may be it would be better to expose unhandled rejections as a event/callback instead of propagating the error as a result. What do you think? But this is subject to a separate issue.

[davojan]

Hey, @laverdet. Hope you are doing well.

This is kindly reminder that this bug is quite severe at least for my use-case. Today I found myself spent several hours to debug a strange behavior and it turned out that it was caused by the same nonsense unhandled promise rejections described in this issue.

Could you please at least express your opinion on this? Is it going to be fixed sometime or it works as intended?

And by the way - thanks for your great work on this library so far!

[laverdet]

Your complaint here is totally valid, and your issue is clear and well-researched. The intent of the API was to have something more approachable than v8's complicated promise lifecycle API, but there's definitely a lot be desired. Anyway, this module is currently doing everything that I personally need it to do, and I don't have the bandwidth to work on changes to this. I am one man, not a company. I'm happy to mentor you, or anyone else, if you want to take the time to research and implement a better approach.

[caracal7]

I have the same problem. And it make difficult to run untrusted code.

@davojan did you found some solutions how to bypass this behaviour?

[davojan]

@davojan did you found some solutions how to bypass this behaviour?

I have a backup plan to override/wrap global Promise class inside the isolate and try to smartly track all async calls and detect leaked promises on javascript side. But I haven't implemented it yet.

Another solution I see is to negotiate with @laverdet or find another qualified C++ developer and fund fixing of this bug, which I guess is not a simple fix at all.

[caracal7]

@davojan thanks for reply.

I tried https://github.com/fulcrumapp/v8-sandbox. This library unmaintained by author and little bit buggy but doesn't have the same problem with promises in my cases.