Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Forward unhandled exceptions to main event loop #105

Merged
merged 2 commits into from

2 participants

@metamatt

Currently, the futures library calls process.exit() if client code throws an exception from a future resolve callback: such errors are always fatal to the whole process. This behavior is more draconian than necessary; instead, as suggested in 95c1b61#commitcomment-2840222, let's forward such exceptions to the uncaughtException handler, so they're neither more nor less fatal than other exceptions.

This PR adds a test for this behavior (which currently fails), and a fix (after which the test works).

metamatt added some commits
@metamatt metamatt Test for exceptions thrown in a future resolve callback,
especially those thrown in a fiber which uses Future.wait,
which has a nested deferred call to Fiber.run which causes
such exceptions to land in the resolver for the previous wait.

This currently exits the whole node process, and should not.
875409f
@metamatt metamatt Forward unhandled exceptions to main event loop
While walking the list of resolve callbacks and calling them, if
something throws an exception, use process.nextTick() to rethrow
the exception from the main event loop.

The reason to catch such exceptions at all is because they're in
client-provided code, and so the fiber-futures library shouldn't
get blamed for them.

The correct destination for such exceptions is the uncaughtException
handler, and that's what rethrowing the exception fron nextTick()
achieves.

The previous behavior was to call process.exit() if such an exception
should happen. That's too severe, however; exceptions in a resolve
callback should be no more or less illegal than exceptions anywhere
else.

Note that in conjunction with Future.wait(), the resolve callback will
often be the local cb() function defined inside Future.wait() itself,
which calls fiber.run(), which propagates any exceptions that should
happen inside the body of the fiber itself: this means it is actually
completely normal for any exceptions that happen inside fiber client
code to land (through fiber.run() and wait()'s cb() routine) in these
callback processing routines' exception handlers.

I don't know whether it's preferable to log the exception at the point
we rethrow it or not. Mostly it will be unwelcome spew, but occasionally
it could be vitally helpful. Also, such logging causes the new test for
this change to fail, because tests in the fibers testsuite are supposed
to print nothing more than "pass" to pass. As a compromise for discussion
I've left the log statement present but commented out.
ddb5a80
@laverdet laverdet merged commit 29aa090 into laverdet:master
@laverdet
Owner

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 21, 2013
  1. @metamatt

    Test for exceptions thrown in a future resolve callback,

    metamatt authored
    especially those thrown in a fiber which uses Future.wait,
    which has a nested deferred call to Fiber.run which causes
    such exceptions to land in the resolver for the previous wait.
    
    This currently exits the whole node process, and should not.
  2. @metamatt

    Forward unhandled exceptions to main event loop

    metamatt authored
    While walking the list of resolve callbacks and calling them, if
    something throws an exception, use process.nextTick() to rethrow
    the exception from the main event loop.
    
    The reason to catch such exceptions at all is because they're in
    client-provided code, and so the fiber-futures library shouldn't
    get blamed for them.
    
    The correct destination for such exceptions is the uncaughtException
    handler, and that's what rethrowing the exception fron nextTick()
    achieves.
    
    The previous behavior was to call process.exit() if such an exception
    should happen. That's too severe, however; exceptions in a resolve
    callback should be no more or less illegal than exceptions anywhere
    else.
    
    Note that in conjunction with Future.wait(), the resolve callback will
    often be the local cb() function defined inside Future.wait() itself,
    which calls fiber.run(), which propagates any exceptions that should
    happen inside the body of the fiber itself: this means it is actually
    completely normal for any exceptions that happen inside fiber client
    code to land (through fiber.run() and wait()'s cb() routine) in these
    callback processing routines' exception handlers.
    
    I don't know whether it's preferable to log the exception at the point
    we rethrow it or not. Mostly it will be unwelcome spew, but occasionally
    it could be vitally helpful. Also, such logging causes the new test for
    this change to fail, because tests in the fibers testsuite are supposed
    to print nothing more than "pass" to pass. As a compromise for discussion
    I've left the log statement present but commented out.
This page is out of date. Refresh to see the latest.
Showing with 60 additions and 4 deletions.
  1. +8 −4 future.js
  2. +52 −0 test/future-exception.js
View
12 future.js
@@ -168,8 +168,10 @@ Future.prototype = {
ref[0](undefined, value);
}
} catch(ex) {
- console.log(String(ex.stack || ex.message || ex));
- process.exit(1);
+ // console.log('Resolve cb threw', String(ex.stack || ex.message || ex));
+ process.nextTick(function() {
+ throw(ex);
+ });
}
}
}
@@ -199,8 +201,10 @@ Future.prototype = {
ref[0](error);
}
} catch(ex) {
- console.log(ex.stack || ex);
- process.exit(1);
+ // console.log('Resolve cb threw', String(ex.stack || ex.message || ex));
+ process.nextTick(function() {
+ throw(ex);
+ });
}
}
}
View
52 test/future-exception.js
@@ -0,0 +1,52 @@
+var Fiber = require('fibers');
+var Future = require('fibers/future');
+
+// Possible outputs:
+// pass: exception is thrown and caught in uncaughtException
+// fail: exception is thrown and not caught
+// no output: process dies
+
+
+var thrown = false;
+var caught = false;
+
+var async = function(continuation) {
+ process.nextTick(function() {
+ continuation();
+ });
+}
+
+process.on('uncaughtException', function(err) {
+ if (err.message === 'Catch me if you can') {
+ caught = true;
+ } else {
+ throw err;
+ }
+});
+
+// This fiber's job is to throw an exception after yielding.
+Fiber(function() {
+ // yield and resume via Future.wait() and its cb() helper
+ var sync = Future.wrap(async)();
+ sync.wait();
+
+ // this should get rethrown to the main event loop
+ thrown = true;
+ throw new Error('Catch me if you can');
+}).run();
+
+// This fiber's job is to make sure the process is still alive after the
+// exception was thrown.
+Fiber(function() {
+ // wait for other fiber to throw exception and yield
+ while (!thrown) {
+ var sync = Future.wrap(async)();
+ sync.wait();
+ }
+
+ // wait once more to allow exception to get caught
+ process.nextTick(function() {
+ // see if we have noticed the exception we expect to
+ console.log(caught ? 'pass' : 'fail');
+ });
+}).run();
Something went wrong with that request. Please try again.