Calling interpreter.run() at wrong time in AsyncFunction does bad things.

[Webifi]

After an asynchronous call is completed in an AsyncFunction, you pass your result back to the interpreter using the supplied callback function, then you need to resume stepping the state stack somehow, often via interpreter.run() if you're not using an external stepper.

For example:

function nativeAsyncFunction(val, callback) {
  setTimeout(() => {
      callback(val)
      interpreter.run() // Continue stepping
    }, 0)
}

If, however, your AsyncFunction ends up not doing an asynchronous call for whatever reason and just executes the callback, if you mistakenly execute interpreter.run() again you'll throw things off, since the existing loop in interpreter.run() was never broken by the asynchronous call.

For example:

function nativeAsyncFunction(val, callback) {
  callback(val)
  interpreter.run() // This will do bad things
}

Sometimes it can be difficult to track whether your AsyncFunction actually did an asynchronous operation or not, especially if it's calling other functions in 3rd party libraries that may or may not end asynchronously.

It would be nice to have run() keep track of its run state. Perhaps by changing the run function to something that will block execution if it's already running, like:

Interpreter.prototype.run = function() {
  if (this.running_) return this.paused_
  this.running_ = true
  try {
    while (!this.paused_ && this.step()) {}
  } catch (e) {
    this.running_ = false
    throw e
  }
  this.running_ = false
  return this.paused_
}

[cpcallen]

This is a good observation.

The original JS Interpreter use-case envisaged that there would always be some external supervisory code responsible for calling .step() (or, less usually, .run()—see for example how Blockly Games Pond runs four simultaneous Interpreter instances by round-robin calls to their respective .step() functions.

I would argue that setTimeout(() => {callback(val); interpreter.run()}, 0) is in a sense incorrect, or at least poor style, because it subverts this expectation. If the callback passed to setTimeout is supposed to cause interpretation to resume, it should do so via a call to some part of the code that created the Interpreter instance in the first place.

However:

• There are many simple use-cases where it would be nice for the embedder just to be able to call .run() once and then not worry about anything after that, and
• It is definitely true that bad things can happen as a result of .step() and .run() being called reentrantly,

so I would agree that some check of this general sort would be very useful. I've re-titled the bug to reflect the broader issue, since this does not affect just AsyncFunctions.

[cpcallen]

Note to anyone tackling this bug: .step probably needs to be split into a wrapper that does the reentrancy check and an implementation (which can also be called from .run) which does not, since .run uses the existing .step as part of its implementation.