Wrong try/catch behavior

[TomiBelan]

The following code doesn't work:

function callme(cb) {
  cb();
}

function main(cb) {
  try {
    callme(cont());
    var result = 0;
    [10, 20, 30].forEach(function (i) {
      result += i;
      throw Error('oh noes');
    });
  }
  catch (e) {
    cb(e);
    return;
  }
  cb(null, result);
}

main(function (e,r) {
  console.log('main() finished with', e, r);
})

Output:

main() finished with [Error: oh noes] undefined
main() finished with [Error: oh noes] undefined
main() finished with [Error: oh noes] undefined
main() finished with null 60

main() "finishes" multiple times (i.e. the console.log callback is called more than one time), which should never happen.

It seems continuation.js messes with the forEach callback and tries to "callback-ize" it even though forEach is always serial (and the function body doesn't contain "cont" nor "obtain"). Perhaps the correct behavior with nested functions is to leave them alone (and transform them separately, depending on whether they use cont) -- though that's just a guess and I'm not sure if that wouldn't break other stuff.

[BYVoid]

Fixed by commit https://github.com/BYVoid/continuation/commit/7e2627e4ea8110ce36e72ab3d8fdf999d00a08bb

[TomiBelan]

Wonderful! However, this code still doesn't work:

function callme(cb) {
  cb();
}

function main(cb) {
  try {
    callme(cont());
    var result = 0;
    [10, 20, 30].forEach(function (i) {
      result += i;
      setTimeout(cont(), 1);
      console.log('after timeout');
      throw Error('oh noes');
    });
  }
  catch (e) {
    cb(e);
    return;
  }
  cb(null, result);
}

main(function (e,r) {
  console.log('main() finished with', e, r);
})

Output:

main() finished with null 60
after timeout
main() finished with [Error: oh noes] undefined
after timeout
main() finished with [Error: oh noes] undefined
after timeout
main() finished with [Error: oh noes] undefined

What happened: The function that's in forEach was split into two: the original, and the continuation that's given to setTimeout. The continuation is not original, so try/catch was incorrectly added to it.

So instead of skipping original function expressions like this: https://github.com/BYVoid/continuation/commit/7e2627e4ea8110ce36e72ab3d8fdf999d00a08bb#L3R56

+      if (node.type === 'FunctionExpression' && node.isContinuation) {

you also need to skip all functions inside them, even if they're generated. traverse() itself probably needs an option to stop descending into them (similar to how options.currentScope works).

(Hmm. Technically, there's no "right" way to handle errors that escape from callbacks or CPS-transformed functions. If this was normal "callback pyramid" code, the exception would bubble up through setTimeout and either appear in the browser console or kill the node.js process. But I think that's probably 'more correct' than exiting from main() multiple times.)

[TomiBelan]

Or perhaps not similar to options.currentScope, but exactly options.currentScope... the uses seem similar.

So, to sketch out a patch:

lib/traverse.js
-      if (node.type === 'FunctionDeclaration' || node.type === 'FunctionExpression') {
+      if ((node.type === 'FunctionDeclaration' || node.type === 'FunctionExpression') && !node.isContinuation) {
lib/syntax/TryStatement.js
-    traverse(tryFunction, function (node) {
-      if (node.type === 'FunctionExpression' && node.isContinuation) {
+    traverse(tryFunction, {currentScope: true}, function (node) {
+      if (node.type === 'FunctionExpression') {

...something along those lines could work. I'll play with this some more later (unless you do it first).

[BYVoid]

I think the code above you wrote works correctly. There is a thing that must be clarified: code written in callback function of Array.forEach runs "in parallel" and can not be joined, because once if you use cont, control flows are passed by explicit continuation function (or "callback function"). If you want to run code in sequencial, you should use for statement.

So I decided not to merge commits you push. If there's still something unclear, fell free to comment.

[TomiBelan]

OK, that test case was dumb. But I still think I'm right, and the following should make my point clearer. Apologies for the length.

Let's say someone is building an "egg timer" web service. Users can click a button that says "Wait 60 seconds" and after 60 seconds an alert pops up. Users can customize what numbers show up on the buttons and store their favorite numbers on the server. A spinner is shown while the page is talking to the server. Here's the code:

var pendingRequests = 0;

function getMyNumbers() {
  pendingRequests++;
  document.getElementById('spinner').style.visible =
      (pendingRequests == 0 ? 'hidden' : 'visible');

  try {
    ajaxRequest('/get_my_favorite_numbers', obtain(numbers));

    numbers.forEach(function (t) {
      var button = document.createElement('button');
      button.innerHTML = "Wait " + t + " seconds";
      button.onclick = function () {
        setTimeout(cont(), t * 1000);
        alwrt(t + " seconds have passed.");
      }
      document.body.appendChild(button);
    });
  } catch (e) {
    pendingRequests--;
    document.getElementById('spinner').style.visible =
        (pendingRequests == 0 ? 'hidden' : 'visible');
    alert("Error downloading your favorite numbers. " + e.message);
    return;
  }
  pendingRequests--;
  document.getElementById('spinner').style.visible =
      (pendingRequests == 0 ? 'hidden' : 'visible');
};

window.onload = getMyNumbers;

getMyNumbers() first downloads the list of favorite numbers, and then it uses forEach to create the "wait" buttons. Each button gets a straightforward onclick handler. You can see that forEach is used correctly here -- though it contains an asynchronous inner function, that's just an onclick handler, and forEach itself is completely synchronous. And if ajaxRequest fails, the exception is caught and the user is alerted.

But wait! The developer made a typo! The onclick handler uses alwrt instead of alert. What happens now?

• If this was made in normal JS: the error would be logged in the error console as usual. Nothing else would happen.
• If the outer function was synchronous (e.g. the developer replaced ajaxRequest(...) with var numbers = [30, 60, 120] because the server code wasn't ready): same as above -- the error goes to the console.
• But with continuation.js: try/catch is added to the inner continuation inside the onclick handler. So when the user clicks the button and waits t seconds, getMyNumbers's catch clause runs and makes a complete mess. Not only does it show a confusing error message (nothing is being downloaded anymore), it also decreases pendingRequests from 0 to -1 so the spinner shows up again.

Though the developer's code is buggy, continuation.js isn't blameless. Unhandled exceptions happen, and the developer didn't mean for the outer catch block to catch this exception and "resurrect" execution of getMyNumbers (and who knows what else, if it also took a callback). Continuation.js shouldn't have added catch (_$err) { _$cont(_$err) } anywhere inside the onclick.

In general, whenever continuation.js finds a function expression or declaration inside a try/catch block, it cannot assume anything about that inner function. It might be synchronous, it might use cont, it might use pyramid style. It might run immediately (like with forEach or map), or it might be an event handler that only runs after the outer function ended (if at all). Ergo, TryStatement shouldn't descend into it at all.

So I think 883c8468230d26bc5fd1abd0ee3d9bb08ee45240 does the right thing. If you feel otherwise, let's discuss further. If I convinced you, but you don't like the test case in 9a048d06f09ea4cd9de9430d3e148776ebdafc4c, feel free to only git merge 883c846 and use a better test case instead (whether the above one or something shorter).

[TomiBelan]

Perhaps you've missed my last comment. Could you please take a look and give me a reply? Thanks.